* [PATCH] ipv6: acquire write lock for addr_list in dev_forward_change
@ 2022-03-15 23:02 Niels Dossche
2022-03-17 11:13 ` Paolo Abeni
0 siblings, 1 reply; 4+ messages in thread
From: Niels Dossche @ 2022-03-15 23:02 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Hideaki YOSHIFUJI, David Ahern, Niels Dossche
No path towards dev_forward_change (common ancestor of paths is in
addrconf_fixup_forwarding) acquires idev->lock for idev->addr_list.
Since addrconf_{join,leave}_anycast acquire a write lock on addr_list in
__ipv6_dev_ac_inc and __ipv6_dev_ac_dec, temporarily unlock when calling
addrconf_{join,leave}_anycast analogous to how it's done in
addrconf_ifdown.
Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
---
net/ipv6/addrconf.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index f908e2fd30b2..4055ded4b7bf 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -818,14 +818,18 @@ static void dev_forward_change(struct inet6_dev *idev)
}
}
+ write_lock_bh(&idev->lock);
list_for_each_entry(ifa, &idev->addr_list, if_list) {
if (ifa->flags&IFA_F_TENTATIVE)
continue;
+ write_unlock_bh(&idev->lock);
if (idev->cnf.forwarding)
addrconf_join_anycast(ifa);
else
addrconf_leave_anycast(ifa);
+ write_lock_bh(&idev->lock);
}
+ write_unlock_bh(&idev->lock);
inet6_netconf_notify_devconf(dev_net(dev), RTM_NEWNETCONF,
NETCONFA_FORWARDING,
dev->ifindex, &idev->cnf);
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ipv6: acquire write lock for addr_list in dev_forward_change
2022-03-15 23:02 [PATCH] ipv6: acquire write lock for addr_list in dev_forward_change Niels Dossche
@ 2022-03-17 11:13 ` Paolo Abeni
2022-03-17 14:45 ` David Ahern
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Abeni @ 2022-03-17 11:13 UTC (permalink / raw)
To: Niels Dossche, netdev, David Ahern; +Cc: David S. Miller, Hideaki YOSHIFUJI
On Wed, 2022-03-16 at 00:02 +0100, Niels Dossche wrote:
> No path towards dev_forward_change (common ancestor of paths is in
> addrconf_fixup_forwarding) acquires idev->lock for idev->addr_list.
> Since addrconf_{join,leave}_anycast acquire a write lock on addr_list in
> __ipv6_dev_ac_inc and __ipv6_dev_ac_dec, temporarily unlock when calling
> addrconf_{join,leave}_anycast analogous to how it's done in
> addrconf_ifdown.
>
> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
> ---
> net/ipv6/addrconf.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index f908e2fd30b2..4055ded4b7bf 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -818,14 +818,18 @@ static void dev_forward_change(struct inet6_dev *idev)
> }
> }
>
> + write_lock_bh(&idev->lock);
> list_for_each_entry(ifa, &idev->addr_list, if_list) {
> if (ifa->flags&IFA_F_TENTATIVE)
> continue;
> + write_unlock_bh(&idev->lock);
This looks weird?!? if 'addr_list' integrity is guaranteed by
idev->lock, than this patch looks incorrect. If addr_list integrity is
ensured elsewhere, why acquiring idev->lock at all?
@David: can you please comment here?
Thanks!
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ipv6: acquire write lock for addr_list in dev_forward_change
2022-03-17 11:13 ` Paolo Abeni
@ 2022-03-17 14:45 ` David Ahern
2022-03-17 14:51 ` Niels Dossche
0 siblings, 1 reply; 4+ messages in thread
From: David Ahern @ 2022-03-17 14:45 UTC (permalink / raw)
To: Paolo Abeni, Niels Dossche, netdev; +Cc: David S. Miller, Hideaki YOSHIFUJI
On 3/17/22 5:13 AM, Paolo Abeni wrote:
> On Wed, 2022-03-16 at 00:02 +0100, Niels Dossche wrote:
>> No path towards dev_forward_change (common ancestor of paths is in
>> addrconf_fixup_forwarding) acquires idev->lock for idev->addr_list.
>> Since addrconf_{join,leave}_anycast acquire a write lock on addr_list in
>> __ipv6_dev_ac_inc and __ipv6_dev_ac_dec, temporarily unlock when calling
>> addrconf_{join,leave}_anycast analogous to how it's done in
>> addrconf_ifdown.
>>
>> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
>> ---
>> net/ipv6/addrconf.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index f908e2fd30b2..4055ded4b7bf 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -818,14 +818,18 @@ static void dev_forward_change(struct inet6_dev *idev)
>> }
>> }
>>
>> + write_lock_bh(&idev->lock);
>> list_for_each_entry(ifa, &idev->addr_list, if_list) {
>> if (ifa->flags&IFA_F_TENTATIVE)
>> continue;
>> + write_unlock_bh(&idev->lock);
>
> This looks weird?!? if 'addr_list' integrity is guaranteed by
> idev->lock, than this patch looks incorrect. If addr_list integrity is
> ensured elsewhere, why acquiring idev->lock at all?
>
> @David: can you please comment here?
>
I have stared at this change a few times. It does look weird and does
not seem to be really solving the problem (or completely solving it).
I think a better option is to investigate moving the locks in the
anycast functions up a layer or two so that the lock here can be held
for the entire list walk.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ipv6: acquire write lock for addr_list in dev_forward_change
2022-03-17 14:45 ` David Ahern
@ 2022-03-17 14:51 ` Niels Dossche
0 siblings, 0 replies; 4+ messages in thread
From: Niels Dossche @ 2022-03-17 14:51 UTC (permalink / raw)
To: David Ahern, Paolo Abeni, netdev; +Cc: David S. Miller, Hideaki YOSHIFUJI
On 17/03/2022 15:45, David Ahern wrote:
> On 3/17/22 5:13 AM, Paolo Abeni wrote:
>> On Wed, 2022-03-16 at 00:02 +0100, Niels Dossche wrote:
>>> No path towards dev_forward_change (common ancestor of paths is in
>>> addrconf_fixup_forwarding) acquires idev->lock for idev->addr_list.
>>> Since addrconf_{join,leave}_anycast acquire a write lock on addr_list in
>>> __ipv6_dev_ac_inc and __ipv6_dev_ac_dec, temporarily unlock when calling
>>> addrconf_{join,leave}_anycast analogous to how it's done in
>>> addrconf_ifdown.
>>>
>>> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
>>> ---
>>> net/ipv6/addrconf.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>>> index f908e2fd30b2..4055ded4b7bf 100644
>>> --- a/net/ipv6/addrconf.c
>>> +++ b/net/ipv6/addrconf.c
>>> @@ -818,14 +818,18 @@ static void dev_forward_change(struct inet6_dev *idev)
>>> }
>>> }
>>>
>>> + write_lock_bh(&idev->lock);
>>> list_for_each_entry(ifa, &idev->addr_list, if_list) {
>>> if (ifa->flags&IFA_F_TENTATIVE)
>>> continue;
>>> + write_unlock_bh(&idev->lock);
>>
>> This looks weird?!? if 'addr_list' integrity is guaranteed by
>> idev->lock, than this patch looks incorrect. If addr_list integrity is
>> ensured elsewhere, why acquiring idev->lock at all?
>>
>> @David: can you please comment here?
>>
>
> I have stared at this change a few times. It does look weird and does
> not seem to be really solving the problem (or completely solving it).
>
> I think a better option is to investigate moving the locks in the
> anycast functions up a layer or two so that the lock here can be held
> for the entire list walk.
Hi
Thanks for the review.
I can write a patch today that fixes it the way you suggest and then send it as a v2.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-17 14:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-15 23:02 [PATCH] ipv6: acquire write lock for addr_list in dev_forward_change Niels Dossche
2022-03-17 11:13 ` Paolo Abeni
2022-03-17 14:45 ` David Ahern
2022-03-17 14:51 ` Niels Dossche
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.