netdev - Re: [PATCH net-next] ipv6: remove RTNL protection from ip6addrlbl

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: <CANn89iLQuN43FFHvSgLLY+2b-Yu2Uhy13tmrY_Q-8zX7zUcPkg@mail.gmail.com>
Date: Wed, 3 Apr 2024 18:25:52 +0200
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] ipv6: remove RTNL protection from ip6addrlbl_dump()

On Wed, Apr 3, 2024 at 6:13 PM David Ahern <dsahern@...nel.org> wrote:
>
> On 4/3/24 6:39 AM, Eric Dumazet wrote:
> > diff --git a/net/ipv6/addrlabel.c b/net/ipv6/addrlabel.c
> > index 17ac45aa7194ce9c148ed95e14dd575d17feeb98..961e853543b64cd2060ef693ae3ad32a44780aa1 100644
> > --- a/net/ipv6/addrlabel.c
> > +++ b/net/ipv6/addrlabel.c
> > @@ -234,7 +234,8 @@ static int __ip6addrlbl_add(struct net *net, struct ip6addrlbl_entry *newp,
> >               hlist_add_head_rcu(&newp->list, &net->ipv6.ip6addrlbl_table.head);
> >  out:
> >       if (!ret)
> > -             net->ipv6.ip6addrlbl_table.seq++;
> > +             WRITE_ONCE(net->ipv6.ip6addrlbl_table.seq,
> > +                        net->ipv6.ip6addrlbl_table.seq + 1);
> >       return ret;
> >  }
> >
> > @@ -445,7 +446,7 @@ static void ip6addrlbl_putmsg(struct nlmsghdr *nlh,
> >  };
> >
> >  static int ip6addrlbl_fill(struct sk_buff *skb,
> > -                        struct ip6addrlbl_entry *p,
> > +                        const struct ip6addrlbl_entry *p,
> >                          u32 lseq,
> >                          u32 portid, u32 seq, int event,
> >                          unsigned int flags)
> > @@ -498,7 +499,7 @@ static int ip6addrlbl_dump(struct sk_buff *skb, struct netlink_callback *cb)
> >       struct net *net = sock_net(skb->sk);
> >       struct ip6addrlbl_entry *p;
> >       int idx = 0, s_idx = cb->args[0];
> > -     int err;
> > +     int err = 0;
> >
> >       if (cb->strict_check) {
> >               err = ip6addrlbl_valid_dump_req(nlh, cb->extack);
> > @@ -510,7 +511,7 @@ static int ip6addrlbl_dump(struct sk_buff *skb, struct netlink_callback *cb)
> >       hlist_for_each_entry_rcu(p, &net->ipv6.ip6addrlbl_table.head, list) {
> >               if (idx >= s_idx) {
> >                       err = ip6addrlbl_fill(skb, p,
> > -                                           net->ipv6.ip6addrlbl_table.seq,
> > +                                           READ_ONCE(net->ipv6.ip6addrlbl_table.seq),
>
> seems like this should be read once on entry, and the same value used
> for all iterations.

I thought of that, but this will miss any update done concurrently (if
all entries fit in a single skb)

It is unclear to me what user space can do with ifal_seq

There is no clear signal like :

Initial seq _before_ the dump, and seq seen _after_ the dump.