| Message ID | 20230724221326.384-2-andrew.kanner@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] drivers: net: use xdp only inside tun_build_skb() | expand |
On Tue, Jul 25, 2023 at 6:15 AM Andrew Kanner <andrew.kanner@gmail.com> wrote: > > Tested with syzkaller repro with reduced packet size. It was > discovered that XDP_PACKET_HEADROOM is not checked in > tun_can_build_skb(), although pad may be incremented in > tun_build_skb(). > > Fixes: 7df13219d757 ("tun: reserve extra headroom only when XDP is set") > Link: https://syzkaller.appspot.com/text?tag=ReproC&x=12b2593ea80000 > Signed-off-by: Andrew Kanner <andrew.kanner@gmail.com> > --- > drivers/net/tun.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 18ccbbe9830a..cdf2bd85b383 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1582,7 +1582,13 @@ static void tun_rx_batched(struct tun_struct *tun, struct tun_file *tfile, > static bool tun_can_build_skb(struct tun_struct *tun, struct tun_file *tfile, > int len, int noblock, bool zerocopy, int *skb_xdp) > { > - if (SKB_DATA_ALIGN(len + TUN_RX_PAD) + > + int pad = TUN_RX_PAD; > + struct bpf_prog *xdp_prog = rcu_dereference(tun->xdp_prog); This misses rcu read lock. I wonder if things could be simpler if we move the limit check from tun_can_build_skb() to tun_build_skb(): rcu_read_lock(); xdp_prog = rcu_dereference(tun->xdp_prog); if (xdp_prog) pad += XDP_PACKET_HEADROOM; buflen += SKB_DATA_ALIGN(len + pad); rcu_read_unlock(); Thanks > + > + if (xdp_prog) > + pad += XDP_PACKET_HEADROOM; > + > + if (SKB_DATA_ALIGN(len + pad) + > SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) > PAGE_SIZE) { > *skb_xdp = 0; > return false; > -- > 2.39.3 >
On Tue, Jul 25, 2023 at 11:39:46AM +0800, Jason Wang wrote: > On Tue, Jul 25, 2023 at 6:15 AM Andrew Kanner <andrew.kanner@gmail.com> wrote: > > > > Tested with syzkaller repro with reduced packet size. It was > > discovered that XDP_PACKET_HEADROOM is not checked in > > tun_can_build_skb(), although pad may be incremented in > > tun_build_skb(). > > > > Fixes: 7df13219d757 ("tun: reserve extra headroom only when XDP is set") > > Link: https://syzkaller.appspot.com/text?tag=ReproC&x=12b2593ea80000 > > Signed-off-by: Andrew Kanner <andrew.kanner@gmail.com> > > --- > > drivers/net/tun.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > index 18ccbbe9830a..cdf2bd85b383 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -1582,7 +1582,13 @@ static void tun_rx_batched(struct tun_struct *tun, struct tun_file *tfile, > > static bool tun_can_build_skb(struct tun_struct *tun, struct tun_file *tfile, > > int len, int noblock, bool zerocopy, int *skb_xdp) > > { > > - if (SKB_DATA_ALIGN(len + TUN_RX_PAD) + > > + int pad = TUN_RX_PAD; > > + struct bpf_prog *xdp_prog = rcu_dereference(tun->xdp_prog); > > This misses rcu read lock. > > I wonder if things could be simpler if we move the limit check from > tun_can_build_skb() to tun_build_skb(): > > rcu_read_lock(); > xdp_prog = rcu_dereference(tun->xdp_prog); > if (xdp_prog) > pad += XDP_PACKET_HEADROOM; > buflen += SKB_DATA_ALIGN(len + pad); > rcu_read_unlock(); > > Thanks > Thanks, I missed the part with rcu read lock for some reason. It's a good idea to move / reduce duplication. Let me think and try to fix according to your comments, I will resend it as v2.
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 18ccbbe9830a..cdf2bd85b383 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1582,7 +1582,13 @@ static void tun_rx_batched(struct tun_struct *tun, struct tun_file *tfile, static bool tun_can_build_skb(struct tun_struct *tun, struct tun_file *tfile, int len, int noblock, bool zerocopy, int *skb_xdp) { - if (SKB_DATA_ALIGN(len + TUN_RX_PAD) + + int pad = TUN_RX_PAD; + struct bpf_prog *xdp_prog = rcu_dereference(tun->xdp_prog); + + if (xdp_prog) + pad += XDP_PACKET_HEADROOM; + + if (SKB_DATA_ALIGN(len + pad) + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) > PAGE_SIZE) { *skb_xdp = 0; return false;
Tested with syzkaller repro with reduced packet size. It was discovered that XDP_PACKET_HEADROOM is not checked in tun_can_build_skb(), although pad may be incremented in tun_build_skb(). Fixes: 7df13219d757 ("tun: reserve extra headroom only when XDP is set") Link: https://syzkaller.appspot.com/text?tag=ReproC&x=12b2593ea80000 Signed-off-by: Andrew Kanner <andrew.kanner@gmail.com> --- drivers/net/tun.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)