| Message ID | 20200225195445.140896.41873.stgit@awfm-01.aw.intel.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 817a68a6584aa08e323c64283fec5ded7be84759 |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-rc] IB/hfi1, qib: Ensure RCU is locked when accessing list | expand |
On Tue, Feb 25, 2020 at 02:54:45PM -0500, Dennis Dalessandro wrote: > The packet handling function, specifically the iteration of the qp list > for mad packet processing misses locking RCU before running through the > list. Not only is this incorrect, but the list_for_each_entry_rcu() call > can not be called with a conditional check for lock dependency. Remedy > this by invoking the rcu lock and unlock around the critical section. > > This brings MAD packet processing in line with what is done for non-MAD > packets. > > Cc: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> > Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com> > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com> > --- > drivers/infiniband/hw/hfi1/verbs.c | 4 +++- > drivers/infiniband/hw/qib/qib_verbs.c | 2 ++ > 2 files changed, 5 insertions(+), 1 deletion(-) Applied to for-next, thanks Jason
On 2/28/2020 11:15 AM, Jason Gunthorpe wrote: > On Tue, Feb 25, 2020 at 02:54:45PM -0500, Dennis Dalessandro wrote: >> The packet handling function, specifically the iteration of the qp list >> for mad packet processing misses locking RCU before running through the >> list. Not only is this incorrect, but the list_for_each_entry_rcu() call >> can not be called with a conditional check for lock dependency. Remedy >> this by invoking the rcu lock and unlock around the critical section. >> >> This brings MAD packet processing in line with what is done for non-MAD >> packets. >> >> Cc: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> >> Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com> >> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com> >> --- >> drivers/infiniband/hw/hfi1/verbs.c | 4 +++- >> drivers/infiniband/hw/qib/qib_verbs.c | 2 ++ >> 2 files changed, 5 insertions(+), 1 deletion(-) > > Applied to for-next, thanks > Maybe it should have went to -rc? -Denny
On Mon, Mar 02, 2020 at 08:14:52AM -0500, Dennis Dalessandro wrote: > On 2/28/2020 11:15 AM, Jason Gunthorpe wrote: > > On Tue, Feb 25, 2020 at 02:54:45PM -0500, Dennis Dalessandro wrote: > > > The packet handling function, specifically the iteration of the qp list > > > for mad packet processing misses locking RCU before running through the > > > list. Not only is this incorrect, but the list_for_each_entry_rcu() call > > > can not be called with a conditional check for lock dependency. Remedy > > > this by invoking the rcu lock and unlock around the critical section. > > > > > > This brings MAD packet processing in line with what is done for non-MAD > > > packets. > > > > > > Cc: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> > > > Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com> > > > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com> > > > drivers/infiniband/hw/hfi1/verbs.c | 4 +++- > > > drivers/infiniband/hw/qib/qib_verbs.c | 2 ++ > > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > Applied to for-next, thanks > > > > Maybe it should have went to -rc? It doesn't even have a fixes line. If you want patches in -rc send better commit message. I keep repeating this again and again.. Jason
On 3/2/2020 8:29 AM, Jason Gunthorpe wrote: > On Mon, Mar 02, 2020 at 08:14:52AM -0500, Dennis Dalessandro wrote: >> On 2/28/2020 11:15 AM, Jason Gunthorpe wrote: >>> On Tue, Feb 25, 2020 at 02:54:45PM -0500, Dennis Dalessandro wrote: >>>> The packet handling function, specifically the iteration of the qp list >>>> for mad packet processing misses locking RCU before running through the >>>> list. Not only is this incorrect, but the list_for_each_entry_rcu() call >>>> can not be called with a conditional check for lock dependency. Remedy >>>> this by invoking the rcu lock and unlock around the critical section. >>>> >>>> This brings MAD packet processing in line with what is done for non-MAD >>>> packets. >>>> >>>> Cc: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> >>>> Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com> >>>> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com> >>>> drivers/infiniband/hw/hfi1/verbs.c | 4 +++- >>>> drivers/infiniband/hw/qib/qib_verbs.c | 2 ++ >>>> 2 files changed, 5 insertions(+), 1 deletion(-) >>> >>> Applied to for-next, thanks >>> >> >> Maybe it should have went to -rc? > > It doesn't even have a fixes line. If you want patches in -rc send > better commit message. I keep repeating this again and again.. > > Jason > Crap, yeah my bad on that. However there really isn't a good fixes line for this. It's pretty much just the initial commit of the driver, does that really help? It's still just in your WIP branch so is it too late to add: Fixes: 7724105686e7 ("IB/hfi1: add driver files") Other than a fixes line what else do you need for the commit message? Messed up locking is pretty self explanatory I would think. -Denny
On Mon, Mar 02, 2020 at 09:52:03AM -0500, Dennis Dalessandro wrote: > On 3/2/2020 8:29 AM, Jason Gunthorpe wrote: > > On Mon, Mar 02, 2020 at 08:14:52AM -0500, Dennis Dalessandro wrote: > > > On 2/28/2020 11:15 AM, Jason Gunthorpe wrote: > > > > On Tue, Feb 25, 2020 at 02:54:45PM -0500, Dennis Dalessandro wrote: > > > > > The packet handling function, specifically the iteration of the qp list > > > > > for mad packet processing misses locking RCU before running through the > > > > > list. Not only is this incorrect, but the list_for_each_entry_rcu() call > > > > > can not be called with a conditional check for lock dependency. Remedy > > > > > this by invoking the rcu lock and unlock around the critical section. > > > > > > > > > > This brings MAD packet processing in line with what is done for non-MAD > > > > > packets. > > > > > > > > > > Cc: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> > > > > > Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com> > > > > > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com> > > > > > drivers/infiniband/hw/hfi1/verbs.c | 4 +++- > > > > > drivers/infiniband/hw/qib/qib_verbs.c | 2 ++ > > > > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > > > > > Applied to for-next, thanks > > > > > > > > > > Maybe it should have went to -rc? > > > > It doesn't even have a fixes line. If you want patches in -rc send > > better commit message. I keep repeating this again and again.. > > > > Jason > > > > Crap, yeah my bad on that. However there really isn't a good fixes line for > this. It's pretty much just the initial commit of the driver, does that > really help? It's still just in your WIP branch so is it too late to add: > > Fixes: 7724105686e7 ("IB/hfi1: add driver files") Yes, it really does help, ok I updated it. > Other than a fixes line what else do you need for the commit message? Messed > up locking is pretty self explanatory I would think. Well, missing rcu_lock only causes problems for realtime kernels, but sure it can be moved to -rc Jason
diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c index 089e201..2f6323a 100644 --- a/drivers/infiniband/hw/hfi1/verbs.c +++ b/drivers/infiniband/hw/hfi1/verbs.c @@ -515,10 +515,11 @@ static inline void hfi1_handle_packet(struct hfi1_packet *packet, opa_get_lid(packet->dlid, 9B)); if (!mcast) goto drop; + rcu_read_lock(); list_for_each_entry_rcu(p, &mcast->qp_list, list) { packet->qp = p->qp; if (hfi1_do_pkey_check(packet)) - goto drop; + goto unlock_drop; spin_lock_irqsave(&packet->qp->r_lock, flags); packet_handler = qp_ok(packet); if (likely(packet_handler)) @@ -527,6 +528,7 @@ static inline void hfi1_handle_packet(struct hfi1_packet *packet, ibp->rvp.n_pkt_drops++; spin_unlock_irqrestore(&packet->qp->r_lock, flags); } + rcu_read_unlock(); /* * Notify rvt_multicast_detach() if it is waiting for us * to finish. diff --git a/drivers/infiniband/hw/qib/qib_verbs.c b/drivers/infiniband/hw/qib/qib_verbs.c index 33778d4..5ef93f8 100644 --- a/drivers/infiniband/hw/qib/qib_verbs.c +++ b/drivers/infiniband/hw/qib/qib_verbs.c @@ -329,8 +329,10 @@ void qib_ib_rcv(struct qib_ctxtdata *rcd, void *rhdr, void *data, u32 tlen) if (mcast == NULL) goto drop; this_cpu_inc(ibp->pmastats->n_multicast_rcv); + rcu_read_lock(); list_for_each_entry_rcu(p, &mcast->qp_list, list) qib_qp_rcv(rcd, hdr, 1, data, tlen, p->qp); + rcu_read_unlock(); /* * Notify rvt_multicast_detach() if it is waiting for us * to finish.