netdev - Re: [PATCHv2 net-next 1/3] bonding: add common function to check ipsec device

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: <a60116a2-bcbd-4d0f-9cfb-7717c188e26f@blackwall.org>
Date: Mon, 19 Aug 2024 11:02:14 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Hangbin Liu <liuhangbin@...il.com>, netdev@...r.kernel.org
Cc: Jay Vosburgh <j.vosburgh@...il.com>,
 "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
 Tariq Toukan <tariqt@...dia.com>, Jianbo Liu <jianbol@...dia.com>,
 Sabrina Dubroca <sd@...asysnail.net>
Subject: Re: [PATCHv2 net-next 1/3] bonding: add common function to check
 ipsec device

On 19/08/2024 10:53, Hangbin Liu wrote:
> This patch adds a common function to check the status of IPSec devices.
> This function will be useful for future implementations, such as IPSec ESN
> and state offload callbacks.
> 
> Suggested-by: Nikolay Aleksandrov <razor@...ckwall.org>
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
>  drivers/net/bonding/bond_main.c | 43 +++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index f9633a6f8571..250a2717b4e9 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -418,6 +418,34 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
>  /*---------------------------------- XFRM -----------------------------------*/
>  
>  #ifdef CONFIG_XFRM_OFFLOAD
> +/**
> + * bond_ipsec_dev - return the device for ipsec offload, or NULL if not exist
> + *                  caller must hold rcu_read_lock.
> + * @xs: pointer to transformer state struct
> + **/
> +static struct net_device bond_ipsec_dev(struct xfrm_state *xs)
> +{
> +	struct net_device *bond_dev = xs->xso.dev;
> +	struct net_device *real_dev;
> +	struct bonding *bond;
> +	struct slave *slave;
> +
> +	if (!bond_dev)
> +		return NULL;
> +
> +	bond = netdev_priv(bond_dev);
> +	slave = rcu_dereference(bond->curr_active_slave);
> +	real_dev = slave ? slave->dev : NULL;
> +
> +	if ((BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) ||
> +	    !slave || !real_dev || !xs->xso.real_dev)
> +		return NULL;

No need to check !slave again here.  !real_dev implies !slave and
vice-versa, if it is set then we must have had a slave.
I prefer the more obvious way - check slave after deref and
bail out, similar to my fix, I think it is easier to follow the
code and more obvious. Although I don't feel strong about that
it's just a preference. :)

> +
> +	WARN_ON(xs->xso.real_dev != slave->dev);
> +
> +	return real_dev;
> +}
> +
>  /**
>   * bond_ipsec_add_sa - program device with a security association
>   * @xs: pointer to transformer state struct
> @@ -595,23 +623,12 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
>   **/
>  static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
>  {
> -	struct net_device *bond_dev = xs->xso.dev;
>  	struct net_device *real_dev;
> -	struct slave *curr_active;
> -	struct bonding *bond;
>  	int err;
>  
> -	bond = netdev_priv(bond_dev);
>  	rcu_read_lock();
> -	curr_active = rcu_dereference(bond->curr_active_slave);
> -	real_dev = curr_active->dev;
> -
> -	if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
> -		err = false;
> -		goto out;
> -	}
> -
> -	if (!xs->xso.real_dev) {
> +	real_dev = bond_ipsec_dev(xs);
> +	if (!real_dev) {
>  		err = false;
>  		goto out;
>  	}