From: Daniel Borkmann <daniel@iogearbox.net>
To: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>,
Alexei Starovoitov <alexei.starovoitov@gmail.com>,
David Miller <davem@davemloft.net>,
Linux Kernel Network Developers <netdev@vger.kernel.org>,
Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Subject: Re: [PATCH net-next 1/1] net_sched: Introduce skbmod action
Date: Wed, 20 Jul 2016 02:23:03 +0200 [thread overview]
Message-ID: <578EC467.2010808@iogearbox.net> (raw)
In-Reply-To: <CAM_iQpWX5HT_yF3guCbQLMo2GSVGA8BTcJ3_bucHrnBrMQbjqw@mail.gmail.com>
On 07/19/2016 08:04 PM, Cong Wang wrote:
> On Tue, Jul 19, 2016 at 8:03 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 07/19/2016 03:56 PM, Jamal Hadi Salim wrote:
>> [...]
>>>>
>>>> But apart from this,
>>>> neither pedit nor tcf_skbmod_run() here handle checksum complete, so
>>>> you'll
>>>> potentially get false positives wrt csum corruption and drops as a result
>>>> when using either of the two.
>>>
>>> pedit maybe tricky. Any suggestions?
>>> On tcf_skbmod_run, mostly ignorance: while doing only ethernet updates;
>>> is it still needed to do the checksum complete?
>>
>> Well, what Cong recently fixed with mirred was related to mac header ...
>>
>> You probably need skb_postpull_rcsum(), skb_postpush_rcsum() pair.
>>
>> Also, what about skb_try_make_writable()?
>
> I don't think so. 1) checksum is supposed to be done by csum action
> rather than pedit (or skbmod if it matters), 2) csum action currently
> already handles that correctly for both egress and ingress: 2a)
> CHECKSUM_COMPLETE is meaningless on egress; 2b) it forces
> CHECKSUM_COMPLETE to be CHECKSUM_NONE on ingress
> and it is correctly respected.
Ahh, right, so it redoes entire csums when worst-case changing one byte
only, since it cannot know what other actions like pedit did previously.
But it also means for your l2 mac addr changes to force a CHECKSUM_COMPLETE
into CHECKSUM_NONE with csum action that you need to call into one of the
l3/l4 helpers as far as I see.
next prev parent reply other threads:[~2016-07-20 0:23 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-17 8:41 [PATCH net-next 1/1] net_sched: Introduce skbmod action Jamal Hadi Salim
2016-07-18 4:19 ` Alexei Starovoitov
2016-07-18 6:51 ` Jamal Hadi Salim
2016-07-18 9:44 ` Daniel Borkmann
2016-07-18 10:07 ` Thomas Graf
2016-07-18 10:26 ` Jamal Hadi Salim
2016-07-18 17:38 ` Cong Wang
2016-07-19 10:28 ` Jamal Hadi Salim
2016-07-18 10:08 ` Jamal Hadi Salim
2016-07-19 13:21 ` Daniel Borkmann
2016-07-19 13:56 ` Jamal Hadi Salim
2016-07-19 15:03 ` Daniel Borkmann
2016-07-19 18:04 ` Cong Wang
2016-07-20 0:23 ` Daniel Borkmann [this message]
2016-07-21 7:27 ` WAS ( " Jamal Hadi Salim
2016-07-21 14:42 ` Daniel Borkmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=578EC467.2010808@iogearbox.net \
--to=daniel@iogearbox.net \
--cc=alexei.starovoitov@gmail.com \
--cc=davem@davemloft.net \
--cc=jhs@mojatatu.com \
--cc=netdev@vger.kernel.org \
--cc=nikolay@cumulusnetworks.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).