netdev - Re: [PATCH net-next 1/4] ipv6: make inet6_fill

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: <CANn89iJDfhJRPta063ujaASOvgvZ_imeBytm0OWsJ_7oKC4txg@mail.gmail.com>
Date: Thu, 7 Mar 2024 07:24:20 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: David Ahern <dsahern@...nel.org>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH net-next 1/4] ipv6: make inet6_fill_ifaddr() lockless

On Thu, Mar 7, 2024 at 12:38 AM David Ahern <dsahern@...nel.org> wrote:
>
> On 3/6/24 8:51 AM, Eric Dumazet wrote:
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index 2f84e6ecf19f48602cadb47bc378c9b5a1cdbf65..480a1f9492b590bb13008cede5ea7dc9c422af67 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -2730,7 +2730,7 @@ int addrconf_prefix_rcv_add_addr(struct net *net, struct net_device *dev,
> >               if (update_lft) {
> >                       ifp->valid_lft = valid_lft;
> >                       ifp->prefered_lft = prefered_lft;
> > -                     ifp->tstamp = now;
> > +                     WRITE_ONCE(ifp->tstamp, now);
>
> There are a lot of instances of ifp->tstamp not annotated with READ_ONCE
> or WRITE_ONCE. Most of them before this function seem to be updated or
> read under rtnl. What's the general mode of operation for this patch?
> e.g., there are tstamp references just above this one in this function
> not modified. Commit message does not describe why some are updated and
> others not.


Writes on objects that are not yet visible to other threads/cpu do not
need a WRITE_ONCE()

ipv6_add_addr() allocates a fresh object, so

ifa->cstamp = ifa->tstamp = jiffies;  // do not need any WRITE_ONCE()


Reads while we own ifa->lock do no need a READ_ONCE()

So check_cleanup_prefix_route() :

  if (time_before(*expires, ifa->tstamp + lifetime * HZ))  // no need
       *expires = ifa->tstamp + lifetime * HZ;   // no need

In ipv6_create_tempaddr()

  age = (now - ifp->tstamp) / HZ; // no need because we hold ifp->lock;

In ipv6_create_tempaddr()

  age = (now - ifp->tstamp) / HZ; // no need, we hold ifp->lock;

  tmp_tstamp = ifp->tstamp; // no need, we hold ifp->lock;

 addrconf_prefix_rcv_add_addr()
  The reads are done under ifp->lock
  The write uses WRITE_ONCE()

I did a full audit and I think I did not miss any READ_ONCE()/WRITE_ONCE()

Of course, this is extra precaution anyway, the race has no impact
other than KCSAN and/or would require a dumb compiler in the first
place.

If I had to explain this in the changelog, I guess I would not do all
these changes, this would be too time consuming.

Thanks !