Message ID | 20181030003515.12075-1-xiyou.wangcong@gmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Series | [net] net: make pskb_trim_rcsum_slow() robust | expand |
On 10/29/2018 05:35 PM, Cong Wang wrote: > Most callers of pskb_trim_rcsum() simply drops the skb when > it fails, however, ip_check_defrag() still continues to pass > the skb up to stack. In that case, we should restore its previous > csum if __pskb_trim() fails. > > Found this during code review. > > Fixes: 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends") > Cc: Eric Dumazet <edumazet@google.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > net/core/skbuff.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 946de0e24c87..5decd6e6d2b6 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -1843,6 +1843,9 @@ EXPORT_SYMBOL(___pskb_trim); > */ > int pskb_trim_rcsum_slow(struct sk_buff *skb, unsigned int len) > { > + __wsum old_csum = skb->csum; > + int ret; > + > if (skb->ip_summed == CHECKSUM_COMPLETE) { > int delta = skb->len - len; > > @@ -1850,7 +1853,10 @@ int pskb_trim_rcsum_slow(struct sk_buff *skb, unsigned int len) > skb_checksum(skb, len, delta, 0), > len); > } > - return __pskb_trim(skb, len); > + ret = __pskb_trim(skb, len); > + if (unlikely(ret)) > + skb->csum = old_csum; Would not it be simpler to set ip_summed to CHECKSUM_NONE (no need to save old_csum) ? > + return ret; > } > EXPORT_SYMBOL(pskb_trim_rcsum_slow); > >
On Mon, Oct 29, 2018 at 7:14 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Would not it be simpler to set ip_summed to CHECKSUM_NONE (no need to save old_csum) ? For !CHECKSUM_COMPLETE, ip_summed should be untouched, right? If you mean only setting to CHECKSUM_NONE for CHECKSUM_COMPLETE case, the end result may not be simpler.
On 10/29/2018 07:21 PM, Cong Wang wrote: > On Mon, Oct 29, 2018 at 7:14 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: >> >> Would not it be simpler to set ip_summed to CHECKSUM_NONE (no need to save old_csum) ? > > For !CHECKSUM_COMPLETE, ip_summed should be untouched, right? > > If you mean only setting to CHECKSUM_NONE for CHECKSUM_COMPLETE case, > the end result may not be simpler. I meant to reinstate what was there before my patch in this error case if (skb->ip_summed == CHECKSUM_COMPLETE) skb->ip_summed = CHECKSUM_NONE; That would only be run in error (quite unlikely) path, instead of saving old_csum in all cases.
On Mon, Oct 29, 2018 at 7:25 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > On 10/29/2018 07:21 PM, Cong Wang wrote: > > On Mon, Oct 29, 2018 at 7:14 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > >> > >> Would not it be simpler to set ip_summed to CHECKSUM_NONE (no need to save old_csum) ? > > > > For !CHECKSUM_COMPLETE, ip_summed should be untouched, right? > > > > If you mean only setting to CHECKSUM_NONE for CHECKSUM_COMPLETE case, > > the end result may not be simpler. > > I meant to reinstate what was there before my patch in this error case > > if (skb->ip_summed == CHECKSUM_COMPLETE) > skb->ip_summed = CHECKSUM_NONE; > > That would only be run in error (quite unlikely) path, instead of saving old_csum in all cases. I know your point, however, I am not sure that is a desired behavior. On failure, I think the whole skb should be restored to its previous state before entering this function, changing it to CHECKSUM_NONE on failure is inconsistent with success case.
On 10/29/2018 07:41 PM, Cong Wang wrote: > On Mon, Oct 29, 2018 at 7:25 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: >> >> >> >> On 10/29/2018 07:21 PM, Cong Wang wrote: >>> On Mon, Oct 29, 2018 at 7:14 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: >>>> >>>> Would not it be simpler to set ip_summed to CHECKSUM_NONE (no need to save old_csum) ? >>> >>> For !CHECKSUM_COMPLETE, ip_summed should be untouched, right? >>> >>> If you mean only setting to CHECKSUM_NONE for CHECKSUM_COMPLETE case, >>> the end result may not be simpler. >> >> I meant to reinstate what was there before my patch in this error case >> >> if (skb->ip_summed == CHECKSUM_COMPLETE) >> skb->ip_summed = CHECKSUM_NONE; >> >> That would only be run in error (quite unlikely) path, instead of saving old_csum in all cases. > > I know your point, however, I am not sure that is a desired behavior. > > On failure, I think the whole skb should be restored to its previous state > before entering this function, changing it to CHECKSUM_NONE on failure > is inconsistent with success case. > Before my patch, we were changing skb->ip_summed to CHECKSUM_NONE, so why suddenly we need to be consistent ? In any case, ip_check_defrag() should really drop this skb, as for other allocation failures (like skb_share_check()), if really we want consistency.
On Mon, Oct 29, 2018 at 8:08 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > On 10/29/2018 07:41 PM, Cong Wang wrote: > > On Mon, Oct 29, 2018 at 7:25 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > >> > >> > >> > >> On 10/29/2018 07:21 PM, Cong Wang wrote: > >>> On Mon, Oct 29, 2018 at 7:14 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > >>>> > >>>> Would not it be simpler to set ip_summed to CHECKSUM_NONE (no need to save old_csum) ? > >>> > >>> For !CHECKSUM_COMPLETE, ip_summed should be untouched, right? > >>> > >>> If you mean only setting to CHECKSUM_NONE for CHECKSUM_COMPLETE case, > >>> the end result may not be simpler. > >> > >> I meant to reinstate what was there before my patch in this error case > >> > >> if (skb->ip_summed == CHECKSUM_COMPLETE) > >> skb->ip_summed = CHECKSUM_NONE; > >> > >> That would only be run in error (quite unlikely) path, instead of saving old_csum in all cases. > > > > I know your point, however, I am not sure that is a desired behavior. > > > > On failure, I think the whole skb should be restored to its previous state > > before entering this function, changing it to CHECKSUM_NONE on failure > > is inconsistent with success case. > > > > Before my patch, we were changing skb->ip_summed to CHECKSUM_NONE, > so why suddenly we need to be consistent ? That is because setting it to CHECKSUM_NONE _was_ how the success case works and nothing _was_ needed for failure case. You changed how we handle checksum for success case, it is why we need to change for the failure case too. > > In any case, ip_check_defrag() should really drop this skb, as for other allocation > failures (like skb_share_check()), if really we want consistency. I have the same feeling, just not brave enough to change the logic of ip_check_defrag() where pskb_may_pull() failure is treated in a same way.
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Mon, 29 Oct 2018 17:35:15 -0700 > Most callers of pskb_trim_rcsum() simply drops the skb when > it fails, however, ip_check_defrag() still continues to pass > the skb up to stack. In that case, we should restore its previous > csum if __pskb_trim() fails. > > Found this during code review. > > Fixes: 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends") > Cc: Eric Dumazet <edumazet@google.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> I kind of agree with Eric that we should make all callers, including ip_check_defrag(), fail just as with any memory allocation failure.
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 946de0e24c87..5decd6e6d2b6 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -1843,6 +1843,9 @@ EXPORT_SYMBOL(___pskb_trim); */ int pskb_trim_rcsum_slow(struct sk_buff *skb, unsigned int len) { + __wsum old_csum = skb->csum; + int ret; + if (skb->ip_summed == CHECKSUM_COMPLETE) { int delta = skb->len - len; @@ -1850,7 +1853,10 @@ int pskb_trim_rcsum_slow(struct sk_buff *skb, unsigned int len) skb_checksum(skb, len, delta, 0), len); } - return __pskb_trim(skb, len); + ret = __pskb_trim(skb, len); + if (unlikely(ret)) + skb->csum = old_csum; + return ret; } EXPORT_SYMBOL(pskb_trim_rcsum_slow);
Most callers of pskb_trim_rcsum() simply drops the skb when it fails, however, ip_check_defrag() still continues to pass the skb up to stack. In that case, we should restore its previous csum if __pskb_trim() fails. Found this during code review. Fixes: 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends") Cc: Eric Dumazet <edumazet@google.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/core/skbuff.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)