qib_lookup_qpn() appears to leak pointer out of rcu_read_unlock()
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 2 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 4 lines · Mike
Quoted 2 lines
So what am I missing here?
The atomic increment of a reference count:
Got it, thank you, apologies for the noise!
Thanx, Paul
Quoted 27 lines
struct
qib_qp*qib_lookup_qpn(structqib_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; }
Quoted 2 lines
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
BTW, I am considering eliminating the atomic_inc() in favor of widening the scope of the rcu lock expanse.
Mike
Quoted 9 lines
-----Original Message-----
From: Paul E. McKenney [mailto:[email protected]]
Sent: Wednesday, February 12, 2014 9:56 AM
To: Marciniszyn, Mike
Cc: [email protected]; Hefty, Sean; [email protected]; linux- [email protected]; [email protected]
Subject: Re:qib_lookup_qpn()appears to leak pointer out ofrcu_read_unlock()On Wed, Feb 12, 2014 at 01:59:30PM +0000, Marciniszyn, Mike wrote:
Quoted 37 lines
Quoted 4 lines
Quoted 2 lines
So what am I missing here?
The atomic increment of a reference count:
Got it, thank you, apologies for the noise!
Thanx, Paul
Quoted 28 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 1 line · Mike
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
Quoted 49 lines
Mike
Quoted 46 lines
-----Original Message-----
From: Paul E. McKenney [mailto:[email protected]]
Sent: Wednesday, February 12, 2014 9:56 AM
To: Marciniszyn, Mike
Cc: [email protected]; Hefty, Sean; [email protected]; linux- [email protected]; [email protected]
Subject: Re:qib_lookup_qpn()appears to leak pointer out ofrcu_read_unlock()Quoted 4 lines · Mike
Quoted 2 lines
So what am I missing here?
The atomic increment of a reference count:
Got it, thank you, apologies for the noise!
Thanx, Paul
Quoted 28 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html