* [PATCH] RDMA/siw: Fix IPv6 addr_list locking
@ 2019-08-26 14:17 Bernard Metzler
2019-08-26 14:25 ` Jason Gunthorpe
2019-08-26 15:12 ` Bernard Metzler
0 siblings, 2 replies; 6+ messages in thread
From: Bernard Metzler @ 2019-08-26 14:17 UTC (permalink / raw)
To: linux-rdma; +Cc: bvanassche, jgg, dledford, Bernard Metzler
Walking the address list of an inet6_dev requires
appropriate locking. Since the called function
siw_listen_address() may sleep, we have to use
rtnl_lock() + rcu_read_lock_bh() instead of
read_lock_bh().
Also introduces checks if we get a device from
in_dev_get(), or in6_dev_get().
Reported-by: Bart Van Assche <bvanassche@acm.org>
Fixes: 6c52fdc244b5 ("rdma/siw: connection management")
Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com>
---
drivers/infiniband/sw/siw/siw_cm.c | 33 +++++++++++++++++++-----------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 1db5ad3d9580..8291bfcde995 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1962,6 +1962,10 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
struct sockaddr_in s_laddr, *s_raddr;
const struct in_ifaddr *ifa;
+ if (!in_dev) {
+ rv = -ENODEV;
+ goto out;
+ }
memcpy(&s_laddr, &id->local_addr, sizeof(s_laddr));
s_raddr = (struct sockaddr_in *)&id->remote_addr;
@@ -1991,22 +1995,26 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
struct sockaddr_in6 *s_laddr = &to_sockaddr_in6(id->local_addr),
*s_raddr = &to_sockaddr_in6(id->remote_addr);
+ if (!in6_dev) {
+ rv = -ENODEV;
+ goto out;
+ }
siw_dbg(id->device,
"laddr %pI6:%d, raddr %pI6:%d\n",
&s_laddr->sin6_addr, ntohs(s_laddr->sin6_port),
&s_raddr->sin6_addr, ntohs(s_raddr->sin6_port));
- read_lock_bh(&in6_dev->lock);
- list_for_each_entry(ifp, &in6_dev->addr_list, if_list) {
- struct sockaddr_in6 bind_addr;
-
+ rtnl_lock();
+ rcu_read_lock_bh();
+ list_for_each_entry_rcu(ifp, &in6_dev->addr_list, if_list) {
if (ipv6_addr_any(&s_laddr->sin6_addr) ||
ipv6_addr_equal(&s_laddr->sin6_addr, &ifp->addr)) {
- bind_addr.sin6_family = AF_INET6;
- bind_addr.sin6_port = s_laddr->sin6_port;
- bind_addr.sin6_flowinfo = 0;
- bind_addr.sin6_addr = ifp->addr;
- bind_addr.sin6_scope_id = dev->ifindex;
+ struct sockaddr_in6 bind_addr = {
+ .sin6_family = AF_INET6,
+ .sin6_port = s_laddr->sin6_port,
+ .sin6_flowinfo = 0,
+ .sin6_addr = ifp->addr,
+ .sin6_scope_id = dev->ifindex };
rv = siw_listen_address(id, backlog,
(struct sockaddr *)&bind_addr,
@@ -2015,12 +2023,13 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
listeners++;
}
}
- read_unlock_bh(&in6_dev->lock);
-
+ rcu_read_unlock_bh();
+ rtnl_unlock();
in6_dev_put(in6_dev);
} else {
- return -EAFNOSUPPORT;
+ rv = -EAFNOSUPPORT;
}
+out:
if (listeners)
rv = 0;
else if (!rv)
--
2.17.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] RDMA/siw: Fix IPv6 addr_list locking
2019-08-26 14:17 [PATCH] RDMA/siw: Fix IPv6 addr_list locking Bernard Metzler
@ 2019-08-26 14:25 ` Jason Gunthorpe
2019-08-26 15:12 ` Bernard Metzler
1 sibling, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2019-08-26 14:25 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, bvanassche, dledford
On Mon, Aug 26, 2019 at 04:17:40PM +0200, Bernard Metzler wrote:
> Walking the address list of an inet6_dev requires
> appropriate locking. Since the called function
> siw_listen_address() may sleep, we have to use
> rtnl_lock() + rcu_read_lock_bh() instead of
> read_lock_bh().
What is the RCU for if you have RTNL?
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] RDMA/siw: Fix IPv6 addr_list locking
2019-08-26 14:17 [PATCH] RDMA/siw: Fix IPv6 addr_list locking Bernard Metzler
2019-08-26 14:25 ` Jason Gunthorpe
@ 2019-08-26 15:12 ` Bernard Metzler
2019-08-26 15:14 ` Jason Gunthorpe
` (2 more replies)
1 sibling, 3 replies; 6+ messages in thread
From: Bernard Metzler @ 2019-08-26 15:12 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma, bvanassche, dledford
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: -----
>To: "Bernard Metzler" <bmt@zurich.ibm.com>
>From: "Jason Gunthorpe" <jgg@ziepe.ca>
>Date: 08/26/2019 04:25PM
>Cc: linux-rdma@vger.kernel.org, bvanassche@acm.org,
>dledford@redhat.com
>Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix IPv6 addr_list locking
>
>On Mon, Aug 26, 2019 at 04:17:40PM +0200, Bernard Metzler wrote:
>> Walking the address list of an inet6_dev requires
>> appropriate locking. Since the called function
>> siw_listen_address() may sleep, we have to use
>> rtnl_lock() + rcu_read_lock_bh() instead of
>> read_lock_bh().
>
>What is the RCU for if you have RTNL?
>
Frankly, I looked around in net/ipv6 and found, if not
rwlocked, addr_list walking to be rcu protected, even
if rtnl_lock()'d (e.g. addrconf_verify_rtnl()).
You are saying this is useless and overdone, since all
changes to that list are rtnl_lock protected right?
I was not sure about that.
For the IPv4 case further up, we also take the rtnl_lock,
and RCU-deref the address pointer (via
in_dev_for_each_ifa_rtnl()).
Hmmm.
Thanks for your help,
Bernard.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] RDMA/siw: Fix IPv6 addr_list locking
2019-08-26 15:12 ` Bernard Metzler
@ 2019-08-26 15:14 ` Jason Gunthorpe
2019-08-26 15:33 ` Bernard Metzler
2019-08-27 16:43 ` Bernard Metzler
2 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2019-08-26 15:14 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, bvanassche, dledford
On Mon, Aug 26, 2019 at 03:12:20PM +0000, Bernard Metzler wrote:
>
> >To: "Bernard Metzler" <bmt@zurich.ibm.com>
> >From: "Jason Gunthorpe" <jgg@ziepe.ca>
> >Date: 08/26/2019 04:25PM
> >Cc: linux-rdma@vger.kernel.org, bvanassche@acm.org,
> >dledford@redhat.com
> >Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix IPv6 addr_list locking
> >
> >On Mon, Aug 26, 2019 at 04:17:40PM +0200, Bernard Metzler wrote:
> >> Walking the address list of an inet6_dev requires
> >> appropriate locking. Since the called function
> >> siw_listen_address() may sleep, we have to use
> >> rtnl_lock() + rcu_read_lock_bh() instead of
> >> read_lock_bh().
> >
> >What is the RCU for if you have RTNL?
> >
>
> Frankly, I looked around in net/ipv6 and found, if not
> rwlocked, addr_list walking to be rcu protected, even
> if rtnl_lock()'d (e.g. addrconf_verify_rtnl()).
>
> You are saying this is useless and overdone, since all
> changes to that list are rtnl_lock protected right?
> I was not sure about that.
I'm not sure, I thought it was the case that rtnl protected the
address lists on write.
> For the IPv4 case further up, we also take the rtnl_lock,
> and RCU-deref the address pointer (via
> in_dev_for_each_ifa_rtnl()).
That uses rtnl_derference which calls into rcu_dereference_protected
which is saying 'this RCU protected data is being read outside RCU
because we are holding the write side lock'
Which means it is locked by RTNL
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: Re: [PATCH] RDMA/siw: Fix IPv6 addr_list locking
2019-08-26 15:12 ` Bernard Metzler
2019-08-26 15:14 ` Jason Gunthorpe
@ 2019-08-26 15:33 ` Bernard Metzler
2019-08-27 16:43 ` Bernard Metzler
2 siblings, 0 replies; 6+ messages in thread
From: Bernard Metzler @ 2019-08-26 15:33 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma, bvanassche, dledford
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: -----
>To: "Bernard Metzler" <BMT@zurich.ibm.com>
>From: "Jason Gunthorpe" <jgg@ziepe.ca>
>Date: 08/26/2019 05:14PM
>Cc: linux-rdma@vger.kernel.org, bvanassche@acm.org,
>dledford@redhat.com
>Subject: [EXTERNAL] Re: Re: [PATCH] RDMA/siw: Fix IPv6 addr_list
>locking
>
>On Mon, Aug 26, 2019 at 03:12:20PM +0000, Bernard Metzler wrote:
>>
>> >To: "Bernard Metzler" <bmt@zurich.ibm.com>
>> >From: "Jason Gunthorpe" <jgg@ziepe.ca>
>> >Date: 08/26/2019 04:25PM
>> >Cc: linux-rdma@vger.kernel.org, bvanassche@acm.org,
>> >dledford@redhat.com
>> >Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix IPv6 addr_list
>locking
>> >
>> >On Mon, Aug 26, 2019 at 04:17:40PM +0200, Bernard Metzler wrote:
>> >> Walking the address list of an inet6_dev requires
>> >> appropriate locking. Since the called function
>> >> siw_listen_address() may sleep, we have to use
>> >> rtnl_lock() + rcu_read_lock_bh() instead of
>> >> read_lock_bh().
>> >
>> >What is the RCU for if you have RTNL?
>> >
>>
>> Frankly, I looked around in net/ipv6 and found, if not
>> rwlocked, addr_list walking to be rcu protected, even
>> if rtnl_lock()'d (e.g. addrconf_verify_rtnl()).
>>
>> You are saying this is useless and overdone, since all
>> changes to that list are rtnl_lock protected right?
>> I was not sure about that.
>
>I'm not sure, I thought it was the case that rtnl protected the
>address lists on write.
>
>> For the IPv4 case further up, we also take the rtnl_lock,
>> and RCU-deref the address pointer (via
>> in_dev_for_each_ifa_rtnl()).
>
>That uses rtnl_derference which calls into rcu_dereference_protected
>which is saying 'this RCU protected data is being read outside RCU
>because we are holding the write side lock'
>
>Which means it is locked by RTNL
>
OK, makes sense.
So this is probably really overdone.
Thanks,
Bernard.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: Re: [PATCH] RDMA/siw: Fix IPv6 addr_list locking
2019-08-26 15:12 ` Bernard Metzler
2019-08-26 15:14 ` Jason Gunthorpe
2019-08-26 15:33 ` Bernard Metzler
@ 2019-08-27 16:43 ` Bernard Metzler
2 siblings, 0 replies; 6+ messages in thread
From: Bernard Metzler @ 2019-08-27 16:43 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma, bvanassche, dledford
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: -----
>To: "Bernard Metzler" <BMT@zurich.ibm.com>
>From: "Jason Gunthorpe" <jgg@ziepe.ca>
>Date: 08/26/2019 05:14PM
>Cc: linux-rdma@vger.kernel.org, bvanassche@acm.org,
>dledford@redhat.com
>Subject: [EXTERNAL] Re: Re: [PATCH] RDMA/siw: Fix IPv6 addr_list
>locking
>
>On Mon, Aug 26, 2019 at 03:12:20PM +0000, Bernard Metzler wrote:
>>
>> >To: "Bernard Metzler" <bmt@zurich.ibm.com>
>> >From: "Jason Gunthorpe" <jgg@ziepe.ca>
>> >Date: 08/26/2019 04:25PM
>> >Cc: linux-rdma@vger.kernel.org, bvanassche@acm.org,
>> >dledford@redhat.com
>> >Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix IPv6 addr_list
>locking
>> >
>> >On Mon, Aug 26, 2019 at 04:17:40PM +0200, Bernard Metzler wrote:
>> >> Walking the address list of an inet6_dev requires
>> >> appropriate locking. Since the called function
>> >> siw_listen_address() may sleep, we have to use
>> >> rtnl_lock() + rcu_read_lock_bh() instead of
>> >> read_lock_bh().
>> >
>> >What is the RCU for if you have RTNL?
>> >
>>
>> Frankly, I looked around in net/ipv6 and found, if not
>> rwlocked, addr_list walking to be rcu protected, even
>> if rtnl_lock()'d (e.g. addrconf_verify_rtnl()).
>>
>> You are saying this is useless and overdone, since all
>> changes to that list are rtnl_lock protected right?
>> I was not sure about that.
>
>I'm not sure, I thought it was the case that rtnl protected the
>address lists on write.
>
>> For the IPv4 case further up, we also take the rtnl_lock,
>> and RCU-deref the address pointer (via
>> in_dev_for_each_ifa_rtnl()).
>
>That uses rtnl_derference which calls into rcu_dereference_protected
>which is saying 'this RCU protected data is being read outside RCU
>because we are holding the write side lock'
>
>Which means it is locked by RTNL
>
Yes, I think you are right - RCU locking is not appropriate,
since we may sleep -- and that's completely against the RCU
principle.
Sorry for the confusion. Will resend the patch.
I will also take the opportunity to add skipping dead
or tentative IPv6 addresses for the wildcard listen
case.
Thanks!
Bernard.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-27 16:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-26 14:17 [PATCH] RDMA/siw: Fix IPv6 addr_list locking Bernard Metzler
2019-08-26 14:25 ` Jason Gunthorpe
2019-08-26 15:12 ` Bernard Metzler
2019-08-26 15:14 ` Jason Gunthorpe
2019-08-26 15:33 ` Bernard Metzler
2019-08-27 16:43 ` Bernard Metzler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).