| Message ID | 20211115163205.1116673-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] net: guard drivers against shared skbs | expand |
On 11/15/21 8:32 AM, Jakub Kicinski wrote: > Commit d8873315065f ("net: add IFF_SKB_TX_SHARED flag to priv_flags") > introduced IFF_SKB_TX_SHARED to protect drivers which are not ready > for getting shared skbs from pktgen sending such frames. > > Some drivers dutifully clear the flag but most don't, even though > they modify the skb or call skb helpers which expect private skbs. > > syzbot has also discovered more sources of shared skbs than just > pktgen (e.g. llc). > > I think defaulting to opt-in is doing more harm than good, those > who care about fast pktgen should inspect their drivers and opt-in. > It's far too risky to enable this flag in ether_setup(). > > Reported-by: syzbot+4c63f36709a642f801c5@syzkaller.appspotmail.com > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > drivers/net/dummy.c | 1 + > net/core/dev.c | 4 ++++ > net/ethernet/eth.c | 1 - > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c > index f82ad7419508..530eaaee2d25 100644 > --- a/drivers/net/dummy.c > +++ b/drivers/net/dummy.c > @@ -123,6 +123,7 @@ static void dummy_setup(struct net_device *dev) > dev->flags |= IFF_NOARP; > dev->flags &= ~IFF_MULTICAST; > dev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE; > + dev->priv_flags |= IFF_TX_SKB_SHARING; > dev->features |= NETIF_F_SG | NETIF_F_FRAGLIST; > dev->features |= NETIF_F_GSO_SOFTWARE; > dev->features |= NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX; > diff --git a/net/core/dev.c b/net/core/dev.c > index 15ac064b5562..476a826bb4f0 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -3661,6 +3661,10 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device > if (unlikely(!skb)) > goto out_null; > > + if (unlikely(skb_shared(skb)) && > + !(dev->priv_flags & IFF_TX_SKB_SHARING)) > + goto out_kfree_skb; So this will break llc, right ? I am sad we are adding so much tests in fast path.
On Mon, 15 Nov 2021 08:56:10 -0800 Eric Dumazet wrote: > On 11/15/21 8:32 AM, Jakub Kicinski wrote: > > Commit d8873315065f ("net: add IFF_SKB_TX_SHARED flag to priv_flags") > > introduced IFF_SKB_TX_SHARED to protect drivers which are not ready > > for getting shared skbs from pktgen sending such frames. > > > > Some drivers dutifully clear the flag but most don't, even though > > they modify the skb or call skb helpers which expect private skbs. > > > > syzbot has also discovered more sources of shared skbs than just > > pktgen (e.g. llc). > > > > I think defaulting to opt-in is doing more harm than good, those > > who care about fast pktgen should inspect their drivers and opt-in. > > It's far too risky to enable this flag in ether_setup(). > > diff --git a/net/core/dev.c b/net/core/dev.c > > index 15ac064b5562..476a826bb4f0 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -3661,6 +3661,10 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device > > if (unlikely(!skb)) > > goto out_null; > > > > + if (unlikely(skb_shared(skb)) && > > + !(dev->priv_flags & IFF_TX_SKB_SHARING)) > > + goto out_kfree_skb; > > So this will break llc, right ? Likely. I haven't checked why LLC thinks it's a good idea to send shared skbs, probably convenience. > I am sad we are adding so much tests in fast path. What's our general stance on shared skbs in the Tx path? If we think that it's okay maybe it's time to turn the BUG_ON(shared_skb)s in pskb functions into return -EINVALs? The IFF_TX_SKB_SHARING flag is pretty toothless as it stands.
On 11/15/21 9:35 AM, Jakub Kicinski wrote: > On Mon, 15 Nov 2021 08:56:10 -0800 Eric Dumazet wrote: >> On 11/15/21 8:32 AM, Jakub Kicinski wrote: >>> Commit d8873315065f ("net: add IFF_SKB_TX_SHARED flag to priv_flags") >>> introduced IFF_SKB_TX_SHARED to protect drivers which are not ready >>> for getting shared skbs from pktgen sending such frames. >>> >>> Some drivers dutifully clear the flag but most don't, even though >>> they modify the skb or call skb helpers which expect private skbs. >>> >>> syzbot has also discovered more sources of shared skbs than just >>> pktgen (e.g. llc). >>> >>> I think defaulting to opt-in is doing more harm than good, those >>> who care about fast pktgen should inspect their drivers and opt-in. >>> It's far too risky to enable this flag in ether_setup(). > >>> diff --git a/net/core/dev.c b/net/core/dev.c >>> index 15ac064b5562..476a826bb4f0 100644 >>> --- a/net/core/dev.c >>> +++ b/net/core/dev.c >>> @@ -3661,6 +3661,10 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device >>> if (unlikely(!skb)) >>> goto out_null; >>> >>> + if (unlikely(skb_shared(skb)) && >>> + !(dev->priv_flags & IFF_TX_SKB_SHARING)) >>> + goto out_kfree_skb; >> >> So this will break llc, right ? > > Likely. I haven't checked why LLC thinks it's a good idea to send > shared skbs, probably convenience. > >> I am sad we are adding so much tests in fast path. > > What's our general stance on shared skbs in the Tx path? If we think > that it's okay maybe it's time to turn the BUG_ON(shared_skb)s in pskb > functions into return -EINVALs? Yes, I think that a WARN_ON_ONCE() should be enough to keep syzbot reports from alerting us, while not crashing regular hosts. > > The IFF_TX_SKB_SHARING flag is pretty toothless as it stands. > skb_padto() needs to be replaced by something better. so that skb can be cloned if needed. static inline int skb_padto(struct sk_buff *skb, unsigned int len) -> static inline struct sk_buff *skb_padto(struct sk_buff *skb, unsigned int len) { }
On Mon, 15 Nov 2021 09:59:56 -0800 Eric Dumazet wrote: > > The IFF_TX_SKB_SHARING flag is pretty toothless as it stands. > > skb_padto() needs to be replaced by something better. > so that skb can be cloned if needed. > > > static inline int skb_padto(struct sk_buff *skb, unsigned int len) > > -> > > static inline struct sk_buff *skb_padto(struct sk_buff *skb, unsigned int len) Indeed, that was my first instinct but I wasn't up for fixing up all the drivers which call skb_pad(), skb_cow_head() etc. Let me leave this be for now..
diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c index f82ad7419508..530eaaee2d25 100644 --- a/drivers/net/dummy.c +++ b/drivers/net/dummy.c @@ -123,6 +123,7 @@ static void dummy_setup(struct net_device *dev) dev->flags |= IFF_NOARP; dev->flags &= ~IFF_MULTICAST; dev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE; + dev->priv_flags |= IFF_TX_SKB_SHARING; dev->features |= NETIF_F_SG | NETIF_F_FRAGLIST; dev->features |= NETIF_F_GSO_SOFTWARE; dev->features |= NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX; diff --git a/net/core/dev.c b/net/core/dev.c index 15ac064b5562..476a826bb4f0 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3661,6 +3661,10 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device if (unlikely(!skb)) goto out_null; + if (unlikely(skb_shared(skb)) && + !(dev->priv_flags & IFF_TX_SKB_SHARING)) + goto out_kfree_skb; + skb = sk_validate_xmit_skb(skb, dev); if (unlikely(!skb)) goto out_null; diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c index c7d9e08107cb..a55a39c77211 100644 --- a/net/ethernet/eth.c +++ b/net/ethernet/eth.c @@ -366,7 +366,6 @@ void ether_setup(struct net_device *dev) dev->addr_len = ETH_ALEN; dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN; dev->flags = IFF_BROADCAST|IFF_MULTICAST; - dev->priv_flags |= IFF_TX_SKB_SHARING; eth_broadcast_addr(dev->broadcast);
Commit d8873315065f ("net: add IFF_SKB_TX_SHARED flag to priv_flags") introduced IFF_SKB_TX_SHARED to protect drivers which are not ready for getting shared skbs from pktgen sending such frames. Some drivers dutifully clear the flag but most don't, even though they modify the skb or call skb helpers which expect private skbs. syzbot has also discovered more sources of shared skbs than just pktgen (e.g. llc). I think defaulting to opt-in is doing more harm than good, those who care about fast pktgen should inspect their drivers and opt-in. It's far too risky to enable this flag in ether_setup(). Reported-by: syzbot+4c63f36709a642f801c5@syzkaller.appspotmail.com Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- drivers/net/dummy.c | 1 + net/core/dev.c | 4 ++++ net/ethernet/eth.c | 1 - 3 files changed, 5 insertions(+), 1 deletion(-)