linux-kernel - Re: [PATCH net 1/2] bonding: fix xfrm offload feature setup on active-backup mode

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: <Z1ulnP7ZleZBlIas@fedora>
Date: Fri, 13 Dec 2024 03:10:20 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Nikolay Aleksandrov <razor@...ckwall.org>
Cc: netdev@...r.kernel.org, Jay Vosburgh <jv@...sburgh.net>,
	Andy Gospodarek <andy@...yhouse.net>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>,
	Andrew Lunn <andrew+netdev@...n.ch>, Shuah Khan <shuah@...nel.org>,
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 1/2] bonding: fix xfrm offload feature setup on
 active-backup mode

On Thu, Dec 12, 2024 at 11:43:15AM +0200, Nikolay Aleksandrov wrote:
> >>> --- a/drivers/net/bonding/bond_netlink.c
> >>> +++ b/drivers/net/bonding/bond_netlink.c
> >>> @@ -568,18 +568,21 @@ static int bond_newlink(struct net *src_net, struct net_device *bond_dev,
> >>>  			struct nlattr *tb[], struct nlattr *data[],
> >>>  			struct netlink_ext_ack *extack)
> >>>  {
> >>> +	struct bonding *bond = netdev_priv(bond_dev);
> >>>  	int err;
> >>>  
> >>> -	err = bond_changelink(bond_dev, tb, data, extack);
> >>> -	if (err < 0)
> >>> +	err = register_netdevice(bond_dev);
> >>> +	if (err)
> >>>  		return err;
> >>>  
> >>> -	err = register_netdevice(bond_dev);
> >>> -	if (!err) {
> >>> -		struct bonding *bond = netdev_priv(bond_dev);
> >>> +	netif_carrier_off(bond_dev);
> >>> +	bond_work_init_all(bond);
> >>>  
> >>> -		netif_carrier_off(bond_dev);
> >>> -		bond_work_init_all(bond);
> >>> +	err = bond_changelink(bond_dev, tb, data, extack);
> >>> +	if (err) {
> >>> +		bond_work_cancel_all(bond);
> >>> +		netif_carrier_on(bond_dev);
> >>
> >> The patch looks good, but I'm curious why the carrier on here?
> > 
> > The current code set netif_carrier_off(bond_dev) after register_netdevice()
> > success, So I make it on if register failed.
> > 
> > Thanks
> > hangbin
> 
> I don't like adding code just for symmetry alone, I think you should drop it
> unless there is an actual reason to turn carrier on.

OK, I will drop it.

Thanks
Hangbin