* [PATCH net-next] ipv6: remove RTNL protection from ip6addrlbl_dump()
@ 2024-04-03 12:39 Eric Dumazet
2024-04-03 16:13 ` David Ahern
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2024-04-03 12:39 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Paolo Abeni, David Ahern
Cc: netdev, eric.dumazet, Eric Dumazet
No longer hold RTNL while calling ip6addrlbl_dump()
("ip addrlabel show")
ip6addrlbl_dump() was already mostly relying on RCU anyway.
Add READ_ONCE()/WRITE_ONCE() annotations around
net->ipv6.ip6addrlbl_table.seq
Also change return value for a completed dump,
so that NLMSG_DONE can be appended to current skb,
saving one recvmsg() system call.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv6/addrlabel.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
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),
NETLINK_CB(cb->skb).portid,
nlh->nlmsg_seq,
RTM_NEWADDRLABEL,
@@ -522,7 +523,7 @@ static int ip6addrlbl_dump(struct sk_buff *skb, struct netlink_callback *cb)
}
rcu_read_unlock();
cb->args[0] = idx;
- return skb->len;
+ return err;
}
static inline int ip6addrlbl_msgsize(void)
@@ -614,7 +615,7 @@ static int ip6addrlbl_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
rcu_read_lock();
p = __ipv6_addr_label(net, addr, ipv6_addr_type(addr), ifal->ifal_index);
- lseq = net->ipv6.ip6addrlbl_table.seq;
+ lseq = READ_ONCE(net->ipv6.ip6addrlbl_table.seq);
if (p)
err = ip6addrlbl_fill(skb, p, lseq,
NETLINK_CB(in_skb).portid,
@@ -647,6 +648,7 @@ int __init ipv6_addr_label_rtnl_register(void)
return ret;
ret = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_GETADDRLABEL,
ip6addrlbl_get,
- ip6addrlbl_dump, RTNL_FLAG_DOIT_UNLOCKED);
+ ip6addrlbl_dump, RTNL_FLAG_DOIT_UNLOCKED |
+ RTNL_FLAG_DUMP_UNLOCKED);
return ret;
}
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] ipv6: remove RTNL protection from ip6addrlbl_dump()
2024-04-03 12:39 [PATCH net-next] ipv6: remove RTNL protection from ip6addrlbl_dump() Eric Dumazet
@ 2024-04-03 16:13 ` David Ahern
2024-04-03 16:25 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: David Ahern @ 2024-04-03 16:13 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: netdev, eric.dumazet
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.
> NETLINK_CB(cb->skb).portid,
> nlh->nlmsg_seq,
> RTM_NEWADDRLABEL,
> @@ -522,7 +523,7 @@ static int ip6addrlbl_dump(struct sk_buff *skb, struct netlink_callback *cb)
> }
> rcu_read_unlock();
> cb->args[0] = idx;
> - return skb->len;
> + return err;
> }
>
> static inline int ip6addrlbl_msgsize(void)
> @@ -614,7 +615,7 @@ static int ip6addrlbl_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>
> rcu_read_lock();
> p = __ipv6_addr_label(net, addr, ipv6_addr_type(addr), ifal->ifal_index);
> - lseq = net->ipv6.ip6addrlbl_table.seq;
> + lseq = READ_ONCE(net->ipv6.ip6addrlbl_table.seq);
> if (p)
> err = ip6addrlbl_fill(skb, p, lseq,
> NETLINK_CB(in_skb).portid,
> @@ -647,6 +648,7 @@ int __init ipv6_addr_label_rtnl_register(void)
> return ret;
> ret = rtnl_register_module(THIS_MODULE, PF_INET6, RTM_GETADDRLABEL,
> ip6addrlbl_get,
> - ip6addrlbl_dump, RTNL_FLAG_DOIT_UNLOCKED);
> + ip6addrlbl_dump, RTNL_FLAG_DOIT_UNLOCKED |
> + RTNL_FLAG_DUMP_UNLOCKED);
> return ret;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] ipv6: remove RTNL protection from ip6addrlbl_dump()
2024-04-03 16:13 ` David Ahern
@ 2024-04-03 16:25 ` Eric Dumazet
2024-04-03 17:49 ` David Ahern
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2024-04-03 16:25 UTC (permalink / raw)
To: David Ahern
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet
On Wed, Apr 3, 2024 at 6:13 PM David Ahern <dsahern@kernel.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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] ipv6: remove RTNL protection from ip6addrlbl_dump()
2024-04-03 16:25 ` Eric Dumazet
@ 2024-04-03 17:49 ` David Ahern
2024-04-03 17:55 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: David Ahern @ 2024-04-03 17:49 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet
On 4/3/24 10:25 AM, Eric Dumazet wrote:
> On Wed, Apr 3, 2024 at 6:13 PM David Ahern <dsahern@kernel.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.
Since this a known change in behavior (however slight), it would be best
to add something in the commit message or a code comment so if someone
hits a problem and does a git bisect the problem and solution are clear.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] ipv6: remove RTNL protection from ip6addrlbl_dump()
2024-04-03 17:49 ` David Ahern
@ 2024-04-03 17:55 ` Eric Dumazet
0 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2024-04-03 17:55 UTC (permalink / raw)
To: David Ahern
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet
On Wed, Apr 3, 2024 at 7:49 PM David Ahern <dsahern@kernel.org> wrote:
> Since this a known change in behavior (however slight), it would be best
> to add something in the commit message or a code comment so if someone
> hits a problem and does a git bisect the problem and solution are clear.
Ok.
The cb->seq / nl_dump_check_consistent() protocol could be implemented later,
if necessary.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-03 17:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-03 12:39 [PATCH net-next] ipv6: remove RTNL protection from ip6addrlbl_dump() Eric Dumazet
2024-04-03 16:13 ` David Ahern
2024-04-03 16:25 ` Eric Dumazet
2024-04-03 17:49 ` David Ahern
2024-04-03 17:55 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox