bonding: Add net_ratelimit for bond_xdp_get_xmit_slave in bond_main.c

[PATCH net v2] bonding: Add net_ratelimit for bond_xdp_get_xmit_slave in bond_main.c

Posted by Jiwon Kim 2 months, 1 week ago

Add net_ratelimit to reduce warnings and logs.
This addresses the WARNING in bond_xdp_get_xmit_slave reported by syzbot.

Setup:
    # Need xdp_tx_prog with return XDP_TX;
    ip l add veth0 type veth peer veth1
    ip l add veth3 type veth peer veth4
    ip l add bond0 type bond mode 6 # <- BOND_MODE_ALB, unsupported by xdp
    ip l add bond1 type bond # <- BOND_MODE_ROUNDROBIN by default
    ip l set veth0 master bond1
    ip l set bond1 up
    ip l set dev bond1 xdpdrv object tx_xdp.o section xdp_tx
    ip l set veth3 master bond0
    ip l set bond0 up
    ip l set veth4 up
    ip l set veth3 xdpgeneric object tx_xdp.o section xdp_tx

Reported-by: syzbot+c187823a52ed505b2257@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=c187823a52ed505b2257
Fixes: 9e2ee5c7e7c3 ("net, bonding: Add XDP support to the bonding driver")
Signed-off-by: Jiwon Kim <jiwonaid0@gmail.com>
---
v2: Change the patch to fix bond_xdp_get_xmit_slave
---
 drivers/net/bonding/bond_main.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b560644ee1b1..91b9cbdcf274 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5610,9 +5610,12 @@ bond_xdp_get_xmit_slave(struct net_device *bond_dev, struct xdp_buff *xdp)
 		break;
 
 	default:
-		/* Should never happen. Mode guarded by bond_xdp_check() */
-		netdev_err(bond_dev, "Unknown bonding mode %d for xdp xmit\n", BOND_MODE(bond));
-		WARN_ON_ONCE(1);
+		/* This might occur when a bond device increases bpf_master_redirect_enabled_key,
+		 * and another bond device with XDP_TX and bond slave.
+		 */
+		if (net_ratelimit())
+			netdev_err(bond_dev, "Unknown bonding mode %d for xdp xmit\n",
+				   BOND_MODE(bond));
 		return NULL;
 	}
 
-- 
2.43.0

Re: [PATCH net v2] bonding: Add net_ratelimit for bond_xdp_get_xmit_slave in bond_main.c

Posted by Nikolay Aleksandrov 2 months, 1 week ago

On 18/09/2024 11:35, Jiwon Kim wrote:
> Add net_ratelimit to reduce warnings and logs.
> This addresses the WARNING in bond_xdp_get_xmit_slave reported by syzbot.
> 

This commit message is severely lacking. I did the heavy lifting and gave you
detailed analysis of the problem, please describe the actual issue and why
this is ok to do. Also the subject is confusing, it should give a concise
summary of what the patch is trying to do and please don't include filenames in it.
You can take a look at other commits for examples.

> Setup:
>     # Need xdp_tx_prog with return XDP_TX;
>     ip l add veth0 type veth peer veth1
>     ip l add veth3 type veth peer veth4
>     ip l add bond0 type bond mode 6 # <- BOND_MODE_ALB, unsupported by xdp
>     ip l add bond1 type bond # <- BOND_MODE_ROUNDROBIN by default
>     ip l set veth0 master bond1
>     ip l set bond1 up
>     ip l set dev bond1 xdpdrv object tx_xdp.o section xdp_tx
>     ip l set veth3 master bond0
>     ip l set bond0 up
>     ip l set veth4 up
>     ip l set veth3 xdpgeneric object tx_xdp.o section xdp_tx

Care to explain why this setup would trigger anything?

> 
> Reported-by: syzbot+c187823a52ed505b2257@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=c187823a52ed505b2257
> Fixes: 9e2ee5c7e7c3 ("net, bonding: Add XDP support to the bonding driver")
> Signed-off-by: Jiwon Kim <jiwonaid0@gmail.com>
> ---
> v2: Change the patch to fix bond_xdp_get_xmit_slave
> ---
>  drivers/net/bonding/bond_main.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index b560644ee1b1..91b9cbdcf274 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5610,9 +5610,12 @@ bond_xdp_get_xmit_slave(struct net_device *bond_dev, struct xdp_buff *xdp)
>  		break;
>  
>  	default:
> -		/* Should never happen. Mode guarded by bond_xdp_check() */
> -		netdev_err(bond_dev, "Unknown bonding mode %d for xdp xmit\n", BOND_MODE(bond));
> -		WARN_ON_ONCE(1);
> +		/* This might occur when a bond device increases bpf_master_redirect_enabled_key,
> +		 * and another bond device with XDP_TX and bond slave.
> +		 */

The comment is confusing and needs to be reworded or dropped altogether.

> +		if (net_ratelimit())
> +			netdev_err(bond_dev, "Unknown bonding mode %d for xdp xmit\n",
> +				   BOND_MODE(bond));
>  		return NULL;
>  	}
>

Re: [PATCH net v2] bonding: Add net_ratelimit for bond_xdp_get_xmit_slave in bond_main.c

Posted by Jiwon Kim 2 months, 1 week ago

On Wed, Sep 18, 2024 at 6:51 PM Nikolay Aleksandrov <razor@blackwall.org> wrote:
>
> On 18/09/2024 11:35, Jiwon Kim wrote:
> > Add net_ratelimit to reduce warnings and logs.
> > This addresses the WARNING in bond_xdp_get_xmit_slave reported by syzbot.
> >
>
> This commit message is severely lacking. I did the heavy lifting and gave you
> detailed analysis of the problem, please describe the actual issue and why
> this is ok to do. Also the subject is confusing, it should give a concise
> summary of what the patch is trying to do and please don't include filenames in it.
> You can take a look at other commits for examples.
>
> > Setup:
> >     # Need xdp_tx_prog with return XDP_TX;
> >     ip l add veth0 type veth peer veth1
> >     ip l add veth3 type veth peer veth4
> >     ip l add bond0 type bond mode 6 # <- BOND_MODE_ALB, unsupported by xdp
> >     ip l add bond1 type bond # <- BOND_MODE_ROUNDROBIN by default
> >     ip l set veth0 master bond1
> >     ip l set bond1 up
> >     ip l set dev bond1 xdpdrv object tx_xdp.o section xdp_tx
> >     ip l set veth3 master bond0
> >     ip l set bond0 up
> >     ip l set veth4 up
> >     ip l set veth3 xdpgeneric object tx_xdp.o section xdp_tx
>
> Care to explain why this setup would trigger anything?
>
> >
> > Reported-by: syzbot+c187823a52ed505b2257@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=c187823a52ed505b2257
> > Fixes: 9e2ee5c7e7c3 ("net, bonding: Add XDP support to the bonding driver")
> > Signed-off-by: Jiwon Kim <jiwonaid0@gmail.com>
> > ---
> > v2: Change the patch to fix bond_xdp_get_xmit_slave
> > ---
> >  drivers/net/bonding/bond_main.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index b560644ee1b1..91b9cbdcf274 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -5610,9 +5610,12 @@ bond_xdp_get_xmit_slave(struct net_device *bond_dev, struct xdp_buff *xdp)
> >               break;
> >
> >       default:
> > -             /* Should never happen. Mode guarded by bond_xdp_check() */
> > -             netdev_err(bond_dev, "Unknown bonding mode %d for xdp xmit\n", BOND_MODE(bond));
> > -             WARN_ON_ONCE(1);
> > +             /* This might occur when a bond device increases bpf_master_redirect_enabled_key,
> > +              * and another bond device with XDP_TX and bond slave.
> > +              */
>
> The comment is confusing and needs to be reworded or dropped altogether.
>
> > +             if (net_ratelimit())
> > +                     netdev_err(bond_dev, "Unknown bonding mode %d for xdp xmit\n",
> > +                                BOND_MODE(bond));
> >               return NULL;
> >       }
> >
>

Hi Nikolay,

I have taken the time to review your feedback and have sent [PATCH net
v3] for your consideration.
Please take a look when you have a moment.

Thank you so much!

Sincerely,

Jiwon Kim