Message ID | CAMDZJNUyaXnVp7jAH-E_d-X__zikoa=-3ypHo64aOnqra7TbRw@mail.gmail.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
> In a case, there is also warn info. The test topo is shown as below. Thanks for testing the patch, Tonghao. > The warn info is shown as below [1]. If we change the CHECKSUM_NONE to > CHECKSUM_UNNECESSARY in the udp4_ufo_fragment(). > and we should add a check in skb_needs_check() when outputting a packet. > > diff --git a/net/core/dev.c b/net/core/dev.c > index 416137c..8fe12a7 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2670,6 +2670,7 @@ static inline bool skb_needs_check(struct > sk_buff *skb, bool tx_path) > { > if (tx_path) > return skb->ip_summed != CHECKSUM_PARTIAL && > + skb->ip_summed != CHECKSUM_UNNECESSARY && > skb->ip_summed != CHECKSUM_NONE; Good catch. Only, the CHECKSUM_NONE case was added specifically to work around this UFO issue on the tx path in commit 6e7bc478c9a0 ("net: skb_needs_check() accepts CHECKSUM_NONE for tx"). If we change the value generated by UFO, we can remove that statement, so + skb->ip_summed != CHECKSUM_UNNECESSARY; - skb->ip_summed != CHECKSUM_NONE; Else the entire check becomes a NOOP. These are the only three valid states on tx. With very few codepaths generating CHECKSUM_UNNECESSARY to begin with, it arguably already is practically a NOOP. I need to look more closely what the statement is intended to protect against, before we relax it even further. The patch will need the same UFO change in ipv6_ufo_fragment. I also had to verify that it is indeed correct to use CHECKSUM_UNNECESSARY for this case. It is not commonly used as alias for CHECKSUM_NONE on the tx path. And indeed does not carry the exact same meaning. It was defined as "no need to checksum" on tx for protocols that skb_checksum_help does not support: http://patchwork.ozlabs.org/patch/146567/ Given that, it should be fine to use in this case.
>> @@ -2670,6 +2670,7 @@ static inline bool skb_needs_check(struct >> sk_buff *skb, bool tx_path) >> { >> if (tx_path) >> return skb->ip_summed != CHECKSUM_PARTIAL && >> + skb->ip_summed != CHECKSUM_UNNECESSARY && >> skb->ip_summed != CHECKSUM_NONE; > > Good catch. Only, the CHECKSUM_NONE case was added specifically to > work around this UFO issue on the tx path in commit 6e7bc478c9a0 > ("net: skb_needs_check() accepts CHECKSUM_NONE for tx"). If we change > the value generated by UFO, we can remove that statement, so > > + skb->ip_summed != CHECKSUM_UNNECESSARY; > - skb->ip_summed != CHECKSUM_NONE; > > Else the entire check becomes a NOOP. These are the only three valid > states on tx. With very few codepaths generating CHECKSUM_UNNECESSARY > to begin with, it arguably already is practically a NOOP. I need to > look more closely what the statement is intended to protect against, > before we relax it even further. On transmit, packets entering skb_gso_segment are expected to always have ip_summed CHECKSUM_PARTIAL. This check was added to track down unexpected exceptions in commit 67fd1a731ff1 ("net: Add debug info to track down GSO checksum bug"). Only when called for the second time, after skb_mac_gso_segment, do we have to possibly handle the case where the GSO layer computes the checksum and changes ip_summed. Since this only goes into 4.11 to 4.13, making two separate skb_needs_check variants for these two call sites seems overkill. I will send the simple fix to convert CHECKSUM_NONE to CHECKSUM_UNNECESSARY. As a side effect of removing UFO in 4.14-rc1, we can also revert commit 6e7bc478c9a0 ("net: skb_needs_check() accepts CHECKSUM_NONE for tx") in net-next.
Thanks, I send a patch, which will revert commit 6e7bc478c9a0 in net-next. On Wed, Aug 9, 2017 at 1:46 AM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: >>> @@ -2670,6 +2670,7 @@ static inline bool skb_needs_check(struct >>> sk_buff *skb, bool tx_path) >>> { >>> if (tx_path) >>> return skb->ip_summed != CHECKSUM_PARTIAL && >>> + skb->ip_summed != CHECKSUM_UNNECESSARY && >>> skb->ip_summed != CHECKSUM_NONE; >> >> Good catch. Only, the CHECKSUM_NONE case was added specifically to >> work around this UFO issue on the tx path in commit 6e7bc478c9a0 >> ("net: skb_needs_check() accepts CHECKSUM_NONE for tx"). If we change >> the value generated by UFO, we can remove that statement, so >> >> + skb->ip_summed != CHECKSUM_UNNECESSARY; >> - skb->ip_summed != CHECKSUM_NONE; >> >> Else the entire check becomes a NOOP. These are the only three valid >> states on tx. With very few codepaths generating CHECKSUM_UNNECESSARY >> to begin with, it arguably already is practically a NOOP. I need to >> look more closely what the statement is intended to protect against, >> before we relax it even further. > > On transmit, packets entering skb_gso_segment are expected to always > have ip_summed CHECKSUM_PARTIAL. This check was added to track down > unexpected exceptions in commit 67fd1a731ff1 ("net: Add debug info to > track down GSO checksum bug"). > > Only when called for the second time, after skb_mac_gso_segment, do we > have to possibly handle the case where the GSO layer computes the > checksum and changes ip_summed. > > Since this only goes into 4.11 to 4.13, making two separate > skb_needs_check variants for these two call sites seems overkill. I > will send the simple fix to convert CHECKSUM_NONE to > CHECKSUM_UNNECESSARY. > > As a side effect of removing UFO in 4.14-rc1, we can also revert > commit 6e7bc478c9a0 ("net: skb_needs_check() accepts CHECKSUM_NONE for > tx") in net-next.
diff --git a/net/core/dev.c b/net/core/dev.c index 416137c..8fe12a7 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2670,6 +2670,7 @@ static inline bool skb_needs_check(struct sk_buff *skb, bool tx_path) { if (tx_path) return skb->ip_summed != CHECKSUM_PARTIAL && + skb->ip_summed != CHECKSUM_UNNECESSARY && skb->ip_summed != CHECKSUM_NONE; return skb->ip_summed == CHECKSUM_NONE; diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index 7812501..0932c85 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -235,7 +235,7 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb, if (uh->check == 0) uh->check = CSUM_MANGLED_0; - skb->ip_summed = CHECKSUM_NONE; + skb->ip_summed = CHECKSUM_UNNECESSARY; /* If there is no outer header we can fake a checksum offload