netdev - Re: [PATCH v2 net-next 01/14] rtnetlink: prepare nla_put

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: <CAD4GDZyV5H4RK_8H2CiUfEj_DSu=w12HqeCzy+2mmu3cMivGww@mail.gmail.com>
Date: Sat, 24 Feb 2024 10:46:22 +0000
From: Donald Hunter <donald.hunter@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, 
	Ido Schimmel <idosch@...dia.com>, Jiri Pirko <jiri@...dia.com>, eric.dumazet@...il.com
Subject: Re: [PATCH v2 net-next 01/14] rtnetlink: prepare nla_put_iflink() to
 run under RCU

On Sat, 24 Feb 2024 at 08:21, Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Fri, Feb 23, 2024 at 4:25 PM Donald Hunter <donald.hunter@...il.com> wrote:
> >
> > I notice that several of the *_get_iflink() implementations are wrapped
> > with rcu_read_lock()/unlock() and many are not. Shouldn't this be done
> > consistently for all?
>
> I do not understand the question, could you give one example of what
> you saw so that I can comment ?

I did include a snippet of your patch showing ipoib_get_iflink() which
does not use rcu_read_lock() / unlock() and vxcan_get_iflink() which
does. Sorry if that wasn't clear. My concern is that I'd expect all
implementers of .ndo_get_iflink to need to be consistent, whether that
is with or without the calls. Does it just mean that individual
drivers are being overly cautious, or are protecting internal usage?

No use of rcu_read_lock() / unlock() here:

> index 7a5be705d71830d5bb3aa26a96a4463df03883a4..6f2a688fccbfb02ae7bdf3d55cca0e77fa9b56b4 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -1272,10 +1272,10 @@ static int ipoib_get_iflink(const struct net_device *dev)
>
>       /* parent interface */
>       if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags))
> -             return dev->ifindex;
> +             return READ_ONCE(dev->ifindex);
>
>       /* child/vlan interface */
> -     return priv->parent->ifindex;
> +     return READ_ONCE(priv->parent->ifindex);
>  }

And use of them here:

> diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
> index 98c669ad5141479b509ee924ddba3da6bca554cd..f7fabba707ea640cab8863e63bb19294e333ba2c 100644
> --- a/drivers/net/can/vxcan.c
> +++ b/drivers/net/can/vxcan.c
> @@ -119,7 +119,7 @@ static int vxcan_get_iflink(const struct net_device *dev)
>
>       rcu_read_lock();
>       peer = rcu_dereference(priv->peer);
> -     iflink = peer ? peer->ifindex : 0;
> +     iflink = peer ? READ_ONCE(peer->ifindex) : 0;
>       rcu_read_unlock();
>
>       return iflink;


> We do not need an rcu_read_lock() only to fetch dev->ifindex, if this
> is what concerns you.

In which case, it seems that no .ndo_get_iflink implementations should
need the rcu_read_* calls?