netdev - Re: neighbour netlink notifications delivered in wrong order

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]

Date:   Tue, 7 Jun 2022 13:03:35 -0700
From:   Francesco Ruggeri <fruggeri@...sta.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     Andy Roulin <aroulin@...dia.com>, netdev@...r.kernel.org
Subject: Re: neighbour netlink notifications delivered in wrong order

On Tue, Jun 7, 2022 at 10:32 AM Stephen Hemminger
<stephen@...workplumber.org> wrote:
>
> On Tue, 7 Jun 2022 09:29:45 -0700
> Francesco Ruggeri <fruggeri@...sta.com> wrote:
>
> > On Mon, Jun 6, 2022 at 8:19 PM Stephen Hemminger
> > <stephen@...workplumber.org> wrote:
> > >
> > > On Mon, 6 Jun 2022 19:07:04 -0700
> > > Andy Roulin <aroulin@...dia.com> wrote:
> > >
> > > > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > > > index 54625287ee5b..a91dfcbfc01c 100644
> > > > --- a/net/core/neighbour.c
> > > > +++ b/net/core/neighbour.c
> > > > @@ -2531,23 +2531,19 @@ static int neigh_fill_info(struct sk_buff *skb,
> > > > struct neighbour *neigh,
> > > >       if (nla_put(skb, NDA_DST, neigh->tbl->key_len, neigh->primary_key))
> > > >               goto nla_put_failure;
> > > >
> > > > -     read_lock_bh(&neigh->lock);
> > > >       ndm->ndm_state   = neigh->nud_state;
> > >
> > > Accessing neighbor state outside of lock is not safe.
> > >
> > > But you should be able to use RCU here??
> >
> > I think the patch removes the lock from neigh_fill_info but it then uses it
> > to protect all calls to neigh_fill_info, so the access should still be safe.
> > In case of __neigh_notify the lock also extends to protect rtnl_notify,
> > guaranteeing that the state cannot be changed while the notification
> > is in progress (I assume all state changes are protected by the same lock).
> > Andy, is that the idea?
>
> Neigh info is already protected by RCU, is per neighbour reader/writer lock
> still needed at all?

The goal of the patch seems to be to make changing a neighbour's state and
delivering the corresponding notification atomic, in order to prevent
reordering of notifications. It uses the existing lock to do so.
Can reordering be prevented if the lock is replaced with rcu?