* [PATCH] RDMA/core: Fix race when resolving IP address @ 2019-06-21 14:09 Dag Moxnes 2019-06-21 14:56 ` Jason Gunthorpe 0 siblings, 1 reply; 3+ messages in thread From: Dag Moxnes @ 2019-06-21 14:09 UTC (permalink / raw) To: dag.moxnes, dledford, jgg, leon; +Cc: linux-rdma, linux-kernel Use neighbour lock when copying MAC address from neighbour data struct in dst_fetch_ha. When not using the lock, it is possible for the function to race with neigh_update, causing it to copy an invalid MAC address. It is possible to provoke this error by calling rdma_resolve_addr in a tight loop, while deleting the corresponding ARP entry in another tight loop. Signed-off-by: Dag Moxnes <dag.moxnes@oracle.com> Change-Id: I3c5f982b304457f0a83ea7def2fac70315ed38b4 --- drivers/infiniband/core/addr.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c index 2f7d141598..e4945fd1bb 100644 --- a/drivers/infiniband/core/addr.c +++ b/drivers/infiniband/core/addr.c @@ -333,12 +333,16 @@ static int dst_fetch_ha(const struct dst_entry *dst, if (!n) return -ENODATA; + read_lock_bh(&n->lock) if (!(n->nud_state & NUD_VALID)) { - neigh_event_send(n, NULL); ret = -ENODATA; } else { memcpy(dev_addr->dst_dev_addr, n->ha, MAX_ADDR_LEN); } + read_unlock_bh(&n->lock); + + if (ret) + neigh_event_send(n, NULL); neigh_release(n); -- 2.20.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] RDMA/core: Fix race when resolving IP address 2019-06-21 14:09 [PATCH] RDMA/core: Fix race when resolving IP address Dag Moxnes @ 2019-06-21 14:56 ` Jason Gunthorpe 2019-06-24 13:40 ` Dag Moxnes 0 siblings, 1 reply; 3+ messages in thread From: Jason Gunthorpe @ 2019-06-21 14:56 UTC (permalink / raw) To: Dag Moxnes; +Cc: dledford, leon, linux-rdma, linux-kernel, Parav Pandit On Fri, Jun 21, 2019 at 04:09:16PM +0200, Dag Moxnes wrote: > Use neighbour lock when copying MAC address from neighbour data struct > in dst_fetch_ha. > > When not using the lock, it is possible for the function to race with > neigh_update, causing it to copy an invalid MAC address. > > It is possible to provoke this error by calling rdma_resolve_addr in a > tight loop, while deleting the corresponding ARP entry in another tight > loop. > > Signed-off-by: Dag Moxnes <dag.moxnes@oracle.com> > Change-Id: I3c5f982b304457f0a83ea7def2fac70315ed38b4 > drivers/infiniband/core/addr.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c > index 2f7d141598..e4945fd1bb 100644 > +++ b/drivers/infiniband/core/addr.c > @@ -333,12 +333,16 @@ static int dst_fetch_ha(const struct dst_entry *dst, > if (!n) > return -ENODATA; > > + read_lock_bh(&n->lock) > if (!(n->nud_state & NUD_VALID)) { > - neigh_event_send(n, NULL); > ret = -ENODATA; > } else { > memcpy(dev_addr->dst_dev_addr, n->ha, MAX_ADDR_LEN); > } > + read_unlock_bh(&n->lock); > + > + if (ret) > + neigh_event_send(n, NULL); > > neigh_release(n); Can we write this with less spaghetti please, maybe: static int dst_fetch_ha(const struct dst_entry *dst, struct rdma_dev_addr *dev_addr, const void *daddr) { struct neighbour *n; int ret = 0; n = dst_neigh_lookup(dst, daddr); if (!n) return -ENODATA; read_lock_bh(&n->lock); if (!(n->nud_state & NUD_VALID)) { read_unlock_bh(&n->lock); goto out_send; } memcpy(dev_addr->dst_dev_addr, n->ha, MAX_ADDR_LEN); read_unlock_bh(&n->lock); goto out_release; out_send: neigh_event_send(n, NULL); ret = -ENODATA; out_release: neigh_release(n); return ret; } Also, Parav should look at it. Thanks, Jason ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] RDMA/core: Fix race when resolving IP address 2019-06-21 14:56 ` Jason Gunthorpe @ 2019-06-24 13:40 ` Dag Moxnes 0 siblings, 0 replies; 3+ messages in thread From: Dag Moxnes @ 2019-06-24 13:40 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: dledford, leon, linux-rdma, linux-kernel, Parav Pandit Hi Jason, Thanks for the review. On 6/21/19 4:56 PM, Jason Gunthorpe wrote: > On Fri, Jun 21, 2019 at 04:09:16PM +0200, Dag Moxnes wrote: >> Use neighbour lock when copying MAC address from neighbour data struct >> in dst_fetch_ha. >> >> When not using the lock, it is possible for the function to race with >> neigh_update, causing it to copy an invalid MAC address. >> >> It is possible to provoke this error by calling rdma_resolve_addr in a >> tight loop, while deleting the corresponding ARP entry in another tight >> loop. >> >> Signed-off-by: Dag Moxnes <dag.moxnes@oracle.com> >> Change-Id: I3c5f982b304457f0a83ea7def2fac70315ed38b4 >> drivers/infiniband/core/addr.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c >> index 2f7d141598..e4945fd1bb 100644 >> +++ b/drivers/infiniband/core/addr.c >> @@ -333,12 +333,16 @@ static int dst_fetch_ha(const struct dst_entry *dst, >> if (!n) >> return -ENODATA; >> >> + read_lock_bh(&n->lock) Miising semicolon at end of statement. Sorry about that. >> if (!(n->nud_state & NUD_VALID)) { >> - neigh_event_send(n, NULL); >> ret = -ENODATA; >> } else { >> memcpy(dev_addr->dst_dev_addr, n->ha, MAX_ADDR_LEN); >> } >> + read_unlock_bh(&n->lock); >> + >> + if (ret) >> + neigh_event_send(n, NULL); >> >> neigh_release(n); > Can we write this with less spaghetti please, maybe: > > static int dst_fetch_ha(const struct dst_entry *dst, > struct rdma_dev_addr *dev_addr, > const void *daddr) > { > struct neighbour *n; > int ret = 0; > > n = dst_neigh_lookup(dst, daddr); > if (!n) > return -ENODATA; > > read_lock_bh(&n->lock); > if (!(n->nud_state & NUD_VALID)) { > read_unlock_bh(&n->lock); > goto out_send; > } > memcpy(dev_addr->dst_dev_addr, n->ha, MAX_ADDR_LEN); > read_unlock_bh(&n->lock); > > goto out_release; > > out_send: > neigh_event_send(n, NULL); > ret = -ENODATA; > out_release: > neigh_release(n); > > return ret; > } Personally I find it more readable when the unlock is done in one place, but sure, I can rewrite it the way you suggest if the reviewers agree that that way is preferable. Regards, -Dag > Also, Parav should look at it. > > Thanks, > Jason ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-24 13:40 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-21 14:09 [PATCH] RDMA/core: Fix race when resolving IP address Dag Moxnes 2019-06-21 14:56 ` Jason Gunthorpe 2019-06-24 13:40 ` Dag Moxnes
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).