* qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
@ 2014-02-12 0:35 ` Paul E. McKenney
0 siblings, 0 replies; 10+ messages in thread
From: Paul E. McKenney @ 2014-02-12 0:35 UTC (permalink / raw)
To: roland-DgEjT+Ai2ygdnm+yROfE0A, sean.hefty-ral2JQCrhuEAvxtiuMwx3w,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Hello!
The qib_lookup_qpn() function does RCU pointer traversals within RCU
read-side critical sections as required, but the qp pointer is returned
from this function after it does rcu_read_unlock(). One of the callers,
qib_rcv_hdrerr(), dereferences this pointer upon return.
This appears to me to be a bug. From what I can see, the structure
pointed to by qp could be freed immediately after the rcu_read_unlock(),
which would result in a SEGV when qib_rcv_hdrerr() does its later
spin_lock(&qp->r_lock).
So what am I missing here?
Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
@ 2014-02-12 0:35 ` Paul E. McKenney
0 siblings, 0 replies; 10+ messages in thread
From: Paul E. McKenney @ 2014-02-12 0:35 UTC (permalink / raw)
To: roland, sean.hefty, hal.rosenstock; +Cc: linux-rdma, linux-kernel
Hello!
The qib_lookup_qpn() function does RCU pointer traversals within RCU
read-side critical sections as required, but the qp pointer is returned
from this function after it does rcu_read_unlock(). One of the callers,
qib_rcv_hdrerr(), dereferences this pointer upon return.
This appears to me to be a bug. From what I can see, the structure
pointed to by qp could be freed immediately after the rcu_read_unlock(),
which would result in a SEGV when qib_rcv_hdrerr() does its later
spin_lock(&qp->r_lock).
So what am I missing here?
Thanx, Paul
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
2014-02-12 0:35 ` Paul E. McKenney
@ 2014-02-12 13:59 ` Marciniszyn, Mike
-1 siblings, 0 replies; 10+ messages in thread
From: Marciniszyn, Mike @ 2014-02-12 13:59 UTC (permalink / raw)
To: paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org,
roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Hefty, Sean,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> So what am I missing here?
>
The atomic increment of a reference count:
struct qib_qp *qib_lookup_qpn(struct qib_ibport *ibp, u32 qpn)
{
struct qib_qp *qp = NULL;
rcu_read_lock();
if (unlikely(qpn <= 1)) {
if (qpn == 0)
qp = rcu_dereference(ibp->qp0);
else
qp = rcu_dereference(ibp->qp1);
if (qp)
atomic_inc(&qp->refcount); <--------------------------
} else {
struct qib_ibdev *dev = &ppd_from_ibp(ibp)->dd->verbs_dev;
unsigned n = qpn_hash(dev, qpn);
for (qp = rcu_dereference(dev->qp_table[n]); qp;
qp = rcu_dereference(qp->next))
if (qp->ibqp.qp_num == qpn) {
atomic_inc(&qp->refcount); <---------------------
break;
}
}
rcu_read_unlock();
return qp;
}
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
@ 2014-02-12 13:59 ` Marciniszyn, Mike
0 siblings, 0 replies; 10+ messages in thread
From: Marciniszyn, Mike @ 2014-02-12 13:59 UTC (permalink / raw)
To: paulmck@linux.vnet.ibm.com, roland@kernel.org, Hefty, Sean,
hal.rosenstock@gmail.com
Cc: linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
> So what am I missing here?
>
The atomic increment of a reference count:
struct qib_qp *qib_lookup_qpn(struct qib_ibport *ibp, u32 qpn)
{
struct qib_qp *qp = NULL;
rcu_read_lock();
if (unlikely(qpn <= 1)) {
if (qpn == 0)
qp = rcu_dereference(ibp->qp0);
else
qp = rcu_dereference(ibp->qp1);
if (qp)
atomic_inc(&qp->refcount); <--------------------------
} else {
struct qib_ibdev *dev = &ppd_from_ibp(ibp)->dd->verbs_dev;
unsigned n = qpn_hash(dev, qpn);
for (qp = rcu_dereference(dev->qp_table[n]); qp;
qp = rcu_dereference(qp->next))
if (qp->ibqp.qp_num == qpn) {
atomic_inc(&qp->refcount); <---------------------
break;
}
}
rcu_read_unlock();
return qp;
}
Mike
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
2014-02-12 13:59 ` Marciniszyn, Mike
@ 2014-02-12 14:55 ` Paul E. McKenney
-1 siblings, 0 replies; 10+ messages in thread
From: Paul E. McKenney @ 2014-02-12 14:55 UTC (permalink / raw)
To: Marciniszyn, Mike
Cc: roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Hefty, Sean,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On Wed, Feb 12, 2014 at 01:59:30PM +0000, Marciniszyn, Mike wrote:
> > So what am I missing here?
> >
>
> The atomic increment of a reference count:
Got it, thank you, apologies for the noise!
Thanx, Paul
> struct qib_qp *qib_lookup_qpn(struct qib_ibport *ibp, u32 qpn)
> {
> struct qib_qp *qp = NULL;
>
> rcu_read_lock();
> if (unlikely(qpn <= 1)) {
> if (qpn == 0)
> qp = rcu_dereference(ibp->qp0);
> else
> qp = rcu_dereference(ibp->qp1);
> if (qp)
> atomic_inc(&qp->refcount); <--------------------------
> } else {
> struct qib_ibdev *dev = &ppd_from_ibp(ibp)->dd->verbs_dev;
> unsigned n = qpn_hash(dev, qpn);
>
> for (qp = rcu_dereference(dev->qp_table[n]); qp;
> qp = rcu_dereference(qp->next))
> if (qp->ibqp.qp_num == qpn) {
> atomic_inc(&qp->refcount); <---------------------
> break;
> }
> }
> rcu_read_unlock();
> return qp;
> }
>
> Mike
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
@ 2014-02-12 14:55 ` Paul E. McKenney
0 siblings, 0 replies; 10+ messages in thread
From: Paul E. McKenney @ 2014-02-12 14:55 UTC (permalink / raw)
To: Marciniszyn, Mike
Cc: roland@kernel.org, Hefty, Sean, hal.rosenstock@gmail.com,
linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
On Wed, Feb 12, 2014 at 01:59:30PM +0000, Marciniszyn, Mike wrote:
> > So what am I missing here?
> >
>
> The atomic increment of a reference count:
Got it, thank you, apologies for the noise!
Thanx, Paul
> struct qib_qp *qib_lookup_qpn(struct qib_ibport *ibp, u32 qpn)
> {
> struct qib_qp *qp = NULL;
>
> rcu_read_lock();
> if (unlikely(qpn <= 1)) {
> if (qpn == 0)
> qp = rcu_dereference(ibp->qp0);
> else
> qp = rcu_dereference(ibp->qp1);
> if (qp)
> atomic_inc(&qp->refcount); <--------------------------
> } else {
> struct qib_ibdev *dev = &ppd_from_ibp(ibp)->dd->verbs_dev;
> unsigned n = qpn_hash(dev, qpn);
>
> for (qp = rcu_dereference(dev->qp_table[n]); qp;
> qp = rcu_dereference(qp->next))
> if (qp->ibqp.qp_num == qpn) {
> atomic_inc(&qp->refcount); <---------------------
> break;
> }
> }
> rcu_read_unlock();
> return qp;
> }
>
> Mike
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
2014-02-12 14:55 ` Paul E. McKenney
@ 2014-02-12 14:57 ` Marciniszyn, Mike
-1 siblings, 0 replies; 10+ messages in thread
From: Marciniszyn, Mike @ 2014-02-12 14:57 UTC (permalink / raw)
To: paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org
Cc: roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Hefty, Sean,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
BTW, I am considering eliminating the atomic_inc() in favor of widening the scope of the rcu lock expanse.
Mike
> -----Original Message-----
> From: Paul E. McKenney [mailto:paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org]
> Sent: Wednesday, February 12, 2014 9:56 AM
> To: Marciniszyn, Mike
> Cc: roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Hefty, Sean; hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
>
> On Wed, Feb 12, 2014 at 01:59:30PM +0000, Marciniszyn, Mike wrote:
> > > So what am I missing here?
> > >
> >
> > The atomic increment of a reference count:
>
> Got it, thank you, apologies for the noise!
>
> Thanx, Paul
>
> > struct qib_qp *qib_lookup_qpn(struct qib_ibport *ibp, u32 qpn) {
> > struct qib_qp *qp = NULL;
> >
> > rcu_read_lock();
> > if (unlikely(qpn <= 1)) {
> > if (qpn == 0)
> > qp = rcu_dereference(ibp->qp0);
> > else
> > qp = rcu_dereference(ibp->qp1);
> > if (qp)
> > atomic_inc(&qp->refcount); <--------------------------
> > } else {
> > struct qib_ibdev *dev = &ppd_from_ibp(ibp)->dd->verbs_dev;
> > unsigned n = qpn_hash(dev, qpn);
> >
> > for (qp = rcu_dereference(dev->qp_table[n]); qp;
> > qp = rcu_dereference(qp->next))
> > if (qp->ibqp.qp_num == qpn) {
> > atomic_inc(&qp->refcount); <---------------------
> > break;
> > }
> > }
> > rcu_read_unlock();
> > return qp;
> > }
> >
> > Mike
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
@ 2014-02-12 14:57 ` Marciniszyn, Mike
0 siblings, 0 replies; 10+ messages in thread
From: Marciniszyn, Mike @ 2014-02-12 14:57 UTC (permalink / raw)
To: paulmck@linux.vnet.ibm.com
Cc: roland@kernel.org, Hefty, Sean, hal.rosenstock@gmail.com,
linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
BTW, I am considering eliminating the atomic_inc() in favor of widening the scope of the rcu lock expanse.
Mike
> -----Original Message-----
> From: Paul E. McKenney [mailto:paulmck@linux.vnet.ibm.com]
> Sent: Wednesday, February 12, 2014 9:56 AM
> To: Marciniszyn, Mike
> Cc: roland@kernel.org; Hefty, Sean; hal.rosenstock@gmail.com; linux-
> rdma@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
>
> On Wed, Feb 12, 2014 at 01:59:30PM +0000, Marciniszyn, Mike wrote:
> > > So what am I missing here?
> > >
> >
> > The atomic increment of a reference count:
>
> Got it, thank you, apologies for the noise!
>
> Thanx, Paul
>
> > struct qib_qp *qib_lookup_qpn(struct qib_ibport *ibp, u32 qpn) {
> > struct qib_qp *qp = NULL;
> >
> > rcu_read_lock();
> > if (unlikely(qpn <= 1)) {
> > if (qpn == 0)
> > qp = rcu_dereference(ibp->qp0);
> > else
> > qp = rcu_dereference(ibp->qp1);
> > if (qp)
> > atomic_inc(&qp->refcount); <--------------------------
> > } else {
> > struct qib_ibdev *dev = &ppd_from_ibp(ibp)->dd->verbs_dev;
> > unsigned n = qpn_hash(dev, qpn);
> >
> > for (qp = rcu_dereference(dev->qp_table[n]); qp;
> > qp = rcu_dereference(qp->next))
> > if (qp->ibqp.qp_num == qpn) {
> > atomic_inc(&qp->refcount); <---------------------
> > break;
> > }
> > }
> > rcu_read_unlock();
> > return qp;
> > }
> >
> > Mike
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
2014-02-12 14:57 ` Marciniszyn, Mike
@ 2014-02-12 21:50 ` Paul E. McKenney
-1 siblings, 0 replies; 10+ messages in thread
From: Paul E. McKenney @ 2014-02-12 21:50 UTC (permalink / raw)
To: Marciniszyn, Mike
Cc: roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Hefty, Sean,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On Wed, Feb 12, 2014 at 02:57:14PM +0000, Marciniszyn, Mike wrote:
> BTW, I am considering eliminating the atomic_inc() in favor of widening the scope of the rcu lock expanse.
As long as the newly included code doesn't block, that should work fine.
(If it does block, another option is SRCU.)
Thanx, Paul
> Mike
>
> > -----Original Message-----
> > From: Paul E. McKenney [mailto:paulmck-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org]
> > Sent: Wednesday, February 12, 2014 9:56 AM
> > To: Marciniszyn, Mike
> > Cc: roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Hefty, Sean; hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; linux-
> > rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
> >
> > On Wed, Feb 12, 2014 at 01:59:30PM +0000, Marciniszyn, Mike wrote:
> > > > So what am I missing here?
> > > >
> > >
> > > The atomic increment of a reference count:
> >
> > Got it, thank you, apologies for the noise!
> >
> > Thanx, Paul
> >
> > > struct qib_qp *qib_lookup_qpn(struct qib_ibport *ibp, u32 qpn) {
> > > struct qib_qp *qp = NULL;
> > >
> > > rcu_read_lock();
> > > if (unlikely(qpn <= 1)) {
> > > if (qpn == 0)
> > > qp = rcu_dereference(ibp->qp0);
> > > else
> > > qp = rcu_dereference(ibp->qp1);
> > > if (qp)
> > > atomic_inc(&qp->refcount); <--------------------------
> > > } else {
> > > struct qib_ibdev *dev = &ppd_from_ibp(ibp)->dd->verbs_dev;
> > > unsigned n = qpn_hash(dev, qpn);
> > >
> > > for (qp = rcu_dereference(dev->qp_table[n]); qp;
> > > qp = rcu_dereference(qp->next))
> > > if (qp->ibqp.qp_num == qpn) {
> > > atomic_inc(&qp->refcount); <---------------------
> > > break;
> > > }
> > > }
> > > rcu_read_unlock();
> > > return qp;
> > > }
> > >
> > > Mike
> > >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
@ 2014-02-12 21:50 ` Paul E. McKenney
0 siblings, 0 replies; 10+ messages in thread
From: Paul E. McKenney @ 2014-02-12 21:50 UTC (permalink / raw)
To: Marciniszyn, Mike
Cc: roland@kernel.org, Hefty, Sean, hal.rosenstock@gmail.com,
linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
On Wed, Feb 12, 2014 at 02:57:14PM +0000, Marciniszyn, Mike wrote:
> BTW, I am considering eliminating the atomic_inc() in favor of widening the scope of the rcu lock expanse.
As long as the newly included code doesn't block, that should work fine.
(If it does block, another option is SRCU.)
Thanx, Paul
> Mike
>
> > -----Original Message-----
> > From: Paul E. McKenney [mailto:paulmck@linux.vnet.ibm.com]
> > Sent: Wednesday, February 12, 2014 9:56 AM
> > To: Marciniszyn, Mike
> > Cc: roland@kernel.org; Hefty, Sean; hal.rosenstock@gmail.com; linux-
> > rdma@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
> >
> > On Wed, Feb 12, 2014 at 01:59:30PM +0000, Marciniszyn, Mike wrote:
> > > > So what am I missing here?
> > > >
> > >
> > > The atomic increment of a reference count:
> >
> > Got it, thank you, apologies for the noise!
> >
> > Thanx, Paul
> >
> > > struct qib_qp *qib_lookup_qpn(struct qib_ibport *ibp, u32 qpn) {
> > > struct qib_qp *qp = NULL;
> > >
> > > rcu_read_lock();
> > > if (unlikely(qpn <= 1)) {
> > > if (qpn == 0)
> > > qp = rcu_dereference(ibp->qp0);
> > > else
> > > qp = rcu_dereference(ibp->qp1);
> > > if (qp)
> > > atomic_inc(&qp->refcount); <--------------------------
> > > } else {
> > > struct qib_ibdev *dev = &ppd_from_ibp(ibp)->dd->verbs_dev;
> > > unsigned n = qpn_hash(dev, qpn);
> > >
> > > for (qp = rcu_dereference(dev->qp_table[n]); qp;
> > > qp = rcu_dereference(qp->next))
> > > if (qp->ibqp.qp_num == qpn) {
> > > atomic_inc(&qp->refcount); <---------------------
> > > break;
> > > }
> > > }
> > > rcu_read_unlock();
> > > return qp;
> > > }
> > >
> > > Mike
> > >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-02-12 21:50 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-12 0:35 qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock() Paul E. McKenney
2014-02-12 0:35 ` Paul E. McKenney
[not found] ` <20140212003511.GA27242-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
2014-02-12 13:59 ` Marciniszyn, Mike
2014-02-12 13:59 ` Marciniszyn, Mike
[not found] ` <32E1700B9017364D9B60AED9960492BC211F3D24-AtyAts71sc9zLByeVOV5+bfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2014-02-12 14:55 ` Paul E. McKenney
2014-02-12 14:55 ` Paul E. McKenney
[not found] ` <20140212145543.GY4250-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
2014-02-12 14:57 ` Marciniszyn, Mike
2014-02-12 14:57 ` Marciniszyn, Mike
[not found] ` <32E1700B9017364D9B60AED9960492BC211F3D75-AtyAts71sc9zLByeVOV5+bfspsVTdybXVpNB7YpNyf8@public.gmane.org>
2014-02-12 21:50 ` Paul E. McKenney
2014-02-12 21:50 ` Paul E. McKenney
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.