netdev - Re: [PATCH net v2] bonding: Add net_ratelimit for bond_xdp_get_xmit_slave in bond

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ebef9a36-d060-4df3-b139-3dda4a84484a@blackwall.org>
Date: Wed, 18 Sep 2024 12:51:06 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Jiwon Kim <jiwonaid0@...il.com>, jv@...sburgh.net, andy@...yhouse.net,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, ast@...nel.org, daniel@...earbox.net, hawk@...nel.org,
 john.fastabend@...il.com, joamaki@...il.com
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 bpf@...r.kernel.org, syzbot+c187823a52ed505b2257@...kaller.appspotmail.com
Subject: Re: [PATCH net v2] bonding: Add net_ratelimit for
 bond_xdp_get_xmit_slave in bond_main.c

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@...kaller.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@...il.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;
>  	}
>