Message ID | f3c4b16c326d117f65a2c591f2e97a9f7ac8def8.1500562424.git.joseph.salisbury@canonical.com |
---|---|
State | New |
Headers | show |
On 30.07.2017 16:42, Joseph Salisbury wrote: > From: Eric Dumazet <edumazet@google.com> > > BugLink: http://bugs.launchpad.net/bugs/1705447 > > Dmitry reported warnings occurring in __skb_gso_segment() [1] > > All SKB_GSO_DODGY producers can allow user space to feed > packets that trigger the current check. > > We could prevent them from doing so, rejecting packets, but > this might add regressions to existing programs. > > It turns out our SKB_GSO_DODGY handlers properly set up checksum > information that is needed anyway when packets needs to be segmented. > > By checking again skb_needs_check() after skb_mac_gso_segment(), > we should remove these pesky warnings, at a very minor cost. > > With help from Willem de Bruijn > > [1] > WARNING: CPU: 1 PID: 6768 at net/core/dev.c:2439 skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434 > lo: caps=(0x000000a2803b7c69, 0x0000000000000000) len=138 data_len=0 gso_size=15883 gso_type=4 ip_summed=0 > Kernel panic - not syncing: panic_on_warn set ... > > CPU: 1 PID: 6768 Comm: syz-executor1 Not tainted 4.9.0 #5 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > ffff8801c063ecd8 ffffffff82346bdf ffffffff00000001 1ffff100380c7d2e > ffffed00380c7d26 0000000041b58ab3 ffffffff84b37e38 ffffffff823468f1 > ffffffff84820740 ffffffff84f289c0 dffffc0000000000 ffff8801c063ee20 > Call Trace: > [<ffffffff82346bdf>] __dump_stack lib/dump_stack.c:15 [inline] > [<ffffffff82346bdf>] dump_stack+0x2ee/0x3ef lib/dump_stack.c:51 > [<ffffffff81827e34>] panic+0x1fb/0x412 kernel/panic.c:179 > [<ffffffff8141f704>] __warn+0x1c4/0x1e0 kernel/panic.c:542 > [<ffffffff8141f7e5>] warn_slowpath_fmt+0xc5/0x100 kernel/panic.c:565 > [<ffffffff8356cbaf>] skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434 > [<ffffffff83585cd2>] __skb_gso_segment+0x482/0x780 net/core/dev.c:2706 > [<ffffffff83586f19>] skb_gso_segment include/linux/netdevice.h:3985 [inline] > [<ffffffff83586f19>] validate_xmit_skb+0x5c9/0xc20 net/core/dev.c:2969 > [<ffffffff835892bb>] __dev_queue_xmit+0xe6b/0x1e70 net/core/dev.c:3383 > [<ffffffff8358a2d7>] dev_queue_xmit+0x17/0x20 net/core/dev.c:3424 > [<ffffffff83ad161d>] packet_snd net/packet/af_packet.c:2930 [inline] > [<ffffffff83ad161d>] packet_sendmsg+0x32ed/0x4d30 net/packet/af_packet.c:2955 > [<ffffffff834f0aaa>] sock_sendmsg_nosec net/socket.c:621 [inline] > [<ffffffff834f0aaa>] sock_sendmsg+0xca/0x110 net/socket.c:631 > [<ffffffff834f329a>] ___sys_sendmsg+0x8fa/0x9f0 net/socket.c:1954 > [<ffffffff834f5e58>] __sys_sendmsg+0x138/0x300 net/socket.c:1988 > [<ffffffff834f604d>] SYSC_sendmsg net/socket.c:1999 [inline] > [<ffffffff834f604d>] SyS_sendmsg+0x2d/0x50 net/socket.c:1995 > [<ffffffff84371941>] entry_SYSCALL_64_fastpath+0x1f/0xc2 > > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Willem de Bruijn <willemb@google.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit b2504a5dbef3305ef41988ad270b0e8ec289331c) > Signed-off-by: Joseph Salisbury <joseph.salisbury@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- The description of the bug report has something that almost looks usable as SRU justification. Just might need the polish of proper template words. -Stefan > net/core/dev.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 54f8c16..f9f7792 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2702,11 +2702,12 @@ static inline bool skb_needs_check(struct sk_buff *skb, bool tx_path) > struct sk_buff *__skb_gso_segment(struct sk_buff *skb, > netdev_features_t features, bool tx_path) > { > + struct sk_buff *segs; > + > if (unlikely(skb_needs_check(skb, tx_path))) { > int err; > > - skb_warn_bad_offload(skb); > - > + /* We're going to init ->check field in TCP or UDP header */ > err = skb_cow_head(skb, 0); > if (err < 0) > return ERR_PTR(err); > @@ -2734,7 +2735,12 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb, > skb_reset_mac_header(skb); > skb_reset_mac_len(skb); > > - return skb_mac_gso_segment(skb, features); > + segs = skb_mac_gso_segment(skb, features); > + > + if (unlikely(skb_needs_check(skb, tx_path))) > + skb_warn_bad_offload(skb); > + > + return segs; > } > EXPORT_SYMBOL(__skb_gso_segment); > >
On 07/30/17 16:42, Joseph Salisbury wrote: > From: Eric Dumazet <edumazet@google.com> > > BugLink: http://bugs.launchpad.net/bugs/1705447 > > Dmitry reported warnings occurring in __skb_gso_segment() [1] > > All SKB_GSO_DODGY producers can allow user space to feed > packets that trigger the current check. > > We could prevent them from doing so, rejecting packets, but > this might add regressions to existing programs. > > It turns out our SKB_GSO_DODGY handlers properly set up checksum > information that is needed anyway when packets needs to be segmented. > > By checking again skb_needs_check() after skb_mac_gso_segment(), > we should remove these pesky warnings, at a very minor cost. > > With help from Willem de Bruijn > > [1] > WARNING: CPU: 1 PID: 6768 at net/core/dev.c:2439 skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434 > lo: caps=(0x000000a2803b7c69, 0x0000000000000000) len=138 data_len=0 gso_size=15883 gso_type=4 ip_summed=0 > Kernel panic - not syncing: panic_on_warn set ... > > CPU: 1 PID: 6768 Comm: syz-executor1 Not tainted 4.9.0 #5 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > ffff8801c063ecd8 ffffffff82346bdf ffffffff00000001 1ffff100380c7d2e > ffffed00380c7d26 0000000041b58ab3 ffffffff84b37e38 ffffffff823468f1 > ffffffff84820740 ffffffff84f289c0 dffffc0000000000 ffff8801c063ee20 > Call Trace: > [<ffffffff82346bdf>] __dump_stack lib/dump_stack.c:15 [inline] > [<ffffffff82346bdf>] dump_stack+0x2ee/0x3ef lib/dump_stack.c:51 > [<ffffffff81827e34>] panic+0x1fb/0x412 kernel/panic.c:179 > [<ffffffff8141f704>] __warn+0x1c4/0x1e0 kernel/panic.c:542 > [<ffffffff8141f7e5>] warn_slowpath_fmt+0xc5/0x100 kernel/panic.c:565 > [<ffffffff8356cbaf>] skb_warn_bad_offload+0x2af/0x390 net/core/dev.c:2434 > [<ffffffff83585cd2>] __skb_gso_segment+0x482/0x780 net/core/dev.c:2706 > [<ffffffff83586f19>] skb_gso_segment include/linux/netdevice.h:3985 [inline] > [<ffffffff83586f19>] validate_xmit_skb+0x5c9/0xc20 net/core/dev.c:2969 > [<ffffffff835892bb>] __dev_queue_xmit+0xe6b/0x1e70 net/core/dev.c:3383 > [<ffffffff8358a2d7>] dev_queue_xmit+0x17/0x20 net/core/dev.c:3424 > [<ffffffff83ad161d>] packet_snd net/packet/af_packet.c:2930 [inline] > [<ffffffff83ad161d>] packet_sendmsg+0x32ed/0x4d30 net/packet/af_packet.c:2955 > [<ffffffff834f0aaa>] sock_sendmsg_nosec net/socket.c:621 [inline] > [<ffffffff834f0aaa>] sock_sendmsg+0xca/0x110 net/socket.c:631 > [<ffffffff834f329a>] ___sys_sendmsg+0x8fa/0x9f0 net/socket.c:1954 > [<ffffffff834f5e58>] __sys_sendmsg+0x138/0x300 net/socket.c:1988 > [<ffffffff834f604d>] SYSC_sendmsg net/socket.c:1999 [inline] > [<ffffffff834f604d>] SyS_sendmsg+0x2d/0x50 net/socket.c:1995 > [<ffffffff84371941>] entry_SYSCALL_64_fastpath+0x1f/0xc2 > > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Willem de Bruijn <willemb@google.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit b2504a5dbef3305ef41988ad270b0e8ec289331c) > Signed-off-by: Joseph Salisbury <joseph.salisbury@canonical.com> Clean cherry-pick, good test results, minor regression potential. Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > net/core/dev.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 54f8c16..f9f7792 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2702,11 +2702,12 @@ static inline bool skb_needs_check(struct sk_buff *skb, bool tx_path) > struct sk_buff *__skb_gso_segment(struct sk_buff *skb, > netdev_features_t features, bool tx_path) > { > + struct sk_buff *segs; > + > if (unlikely(skb_needs_check(skb, tx_path))) { > int err; > > - skb_warn_bad_offload(skb); > - > + /* We're going to init ->check field in TCP or UDP header */ > err = skb_cow_head(skb, 0); > if (err < 0) > return ERR_PTR(err); > @@ -2734,7 +2735,12 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb, > skb_reset_mac_header(skb); > skb_reset_mac_len(skb); > > - return skb_mac_gso_segment(skb, features); > + segs = skb_mac_gso_segment(skb, features); > + > + if (unlikely(skb_needs_check(skb, tx_path))) > + skb_warn_bad_offload(skb); > + > + return segs; > } > EXPORT_SYMBOL(__skb_gso_segment); > >
diff --git a/net/core/dev.c b/net/core/dev.c index 54f8c16..f9f7792 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2702,11 +2702,12 @@ static inline bool skb_needs_check(struct sk_buff *skb, bool tx_path) struct sk_buff *__skb_gso_segment(struct sk_buff *skb, netdev_features_t features, bool tx_path) { + struct sk_buff *segs; + if (unlikely(skb_needs_check(skb, tx_path))) { int err; - skb_warn_bad_offload(skb); - + /* We're going to init ->check field in TCP or UDP header */ err = skb_cow_head(skb, 0); if (err < 0) return ERR_PTR(err); @@ -2734,7 +2735,12 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb, skb_reset_mac_header(skb); skb_reset_mac_len(skb); - return skb_mac_gso_segment(skb, features); + segs = skb_mac_gso_segment(skb, features); + + if (unlikely(skb_needs_check(skb, tx_path))) + skb_warn_bad_offload(skb); + + return segs; } EXPORT_SYMBOL(__skb_gso_segment);