[RESEND] scsi: libfc: Use refcount_* APIs for reference count management
Commit Message
The atomic_t API based object reference counter management is prone to
counter value overflows, object use-after-free issues and to return
puzzling values. The improved refcount_t APIs are designed to address
these known issues with atomic_t reference counter management. This
white paper [1] has detailed reasons for moving from atomic_t to
refcount_t APIs. Hence replace the atomic_* based implementation by its
refcount_* based equivalent.
The issue is identified using atomic_as_refcounter.cocci Coccinelle
semantic patch script.
[1] https://arxiv.org/pdf/1710.06175.pdf
Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Note: The proposal is compile tested only.
Resending the patch for review and feedback.
drivers/scsi/libfc/fc_exch.c | 10 +++++-----
include/scsi/libfc.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
Comments
On Thu, 2023-03-02 at 00:32 +0530, Deepak R Varma wrote:
> The atomic_t API based object reference counter management is prone
> to counter value overflows, object use-after-free issues and to
> return puzzling values. The improved refcount_t APIs are designed to
> address these known issues with atomic_t reference counter
> management. This white paper [1] has detailed reasons for moving from
> atomic_t to refcount_t APIs. Hence replace the atomic_* based
> implementation by its refcount_* based equivalent.
> The issue is identified using atomic_as_refcounter.cocci Coccinelle
> semantic patch script.
>
> [1] https://arxiv.org/pdf/1710.06175.pdf
Citing long whitepapers in support of a patch isn't helpful to time
pressed reviewers, particularly when it's evident you didn't understand
the paper you cite. The argument in the paper for replacing atomics
with refcounts can be summarized as: if a user can cause a counter
overflow in an atomic_t simply by performing some action from userspace
then that represents a source of potential overflow attacks on the
kernel which should be mitigated by replacing the atomic_t in question
with a refcount_t which is overflow resistant.
What's missing from the quoted changelog is a justification of how a
user could cause an overflow in the ex_refcnt atomic_t.
James
On Wed, Mar 01, 2023 at 02:28:49PM -0500, James Bottomley wrote:
> On Thu, 2023-03-02 at 00:32 +0530, Deepak R Varma wrote:
> > The atomic_t API based object reference counter management is prone
> > to counter value overflows, object use-after-free issues and to
> > return puzzling values. The improved refcount_t APIs are designed to
> > address these known issues with atomic_t reference counter
> > management. This white paper [1] has detailed reasons for moving from
> > atomic_t to refcount_t APIs. Hence replace the atomic_* based
> > implementation by its refcount_* based equivalent.
> > The issue is identified using atomic_as_refcounter.cocci Coccinelle
> > semantic patch script.
> >
> > [1] https://arxiv.org/pdf/1710.06175.pdf
>
> Citing long whitepapers in support of a patch isn't helpful to time
> pressed reviewers, particularly when it's evident you didn't understand
> the paper you cite. The argument in the paper for replacing atomics
> with refcounts can be summarized as: if a user can cause a counter
> overflow in an atomic_t simply by performing some action from userspace
> then that represents a source of potential overflow attacks on the
> kernel which should be mitigated by replacing the atomic_t in question
> with a refcount_t which is overflow resistant.
>
> What's missing from the quoted changelog is a justification of how a
> user could cause an overflow in the ex_refcnt atomic_t.
Thank you very much James for the review comments. I truly appreciate your time
and guidance. I will study your feedback and send in a revision with necessary
update to patch log.
Regards,
./drv
>
> James
>
@@ -246,7 +246,7 @@ static const char *fc_exch_rctl_name(unsigned int op)
*/
static inline void fc_exch_hold(struct fc_exch *ep)
{
- atomic_inc(&ep->ex_refcnt);
+ refcount_inc(&ep->ex_refcnt);
}
/**
@@ -312,7 +312,7 @@ static void fc_exch_release(struct fc_exch *ep)
{
struct fc_exch_mgr *mp;
- if (atomic_dec_and_test(&ep->ex_refcnt)) {
+ if (refcount_dec_and_test(&ep->ex_refcnt)) {
mp = ep->em;
if (ep->destructor)
ep->destructor(&ep->seq, ep->arg);
@@ -329,7 +329,7 @@ static inline void fc_exch_timer_cancel(struct fc_exch *ep)
{
if (cancel_delayed_work(&ep->timeout_work)) {
FC_EXCH_DBG(ep, "Exchange timer canceled\n");
- atomic_dec(&ep->ex_refcnt); /* drop hold for timer */
+ refcount_dec(&ep->ex_refcnt); /* drop hold for timer */
}
}
@@ -1897,7 +1897,7 @@ static void fc_exch_reset(struct fc_exch *ep)
ep->state |= FC_EX_RST_CLEANUP;
fc_exch_timer_cancel(ep);
if (ep->esb_stat & ESB_ST_REC_QUAL)
- atomic_dec(&ep->ex_refcnt); /* drop hold for rec_qual */
+ refcount_dec(&ep->ex_refcnt); /* drop hold for rec_qual */
ep->esb_stat &= ~ESB_ST_REC_QUAL;
sp = &ep->seq;
rc = fc_exch_done_locked(ep);
@@ -2332,7 +2332,7 @@ static void fc_exch_els_rrq(struct fc_frame *fp)
*/
if (ep->esb_stat & ESB_ST_REC_QUAL) {
ep->esb_stat &= ~ESB_ST_REC_QUAL;
- atomic_dec(&ep->ex_refcnt); /* drop hold for rec qual */
+ refcount_dec(&ep->ex_refcnt); /* drop hold for rec qual */
}
if (ep->esb_stat & ESB_ST_COMPLETE)
fc_exch_timer_cancel(ep);
@@ -432,7 +432,7 @@ struct fc_seq {
*/
struct fc_exch {
spinlock_t ex_lock;
- atomic_t ex_refcnt;
+ refcount_t ex_refcnt;
enum fc_class class;
struct fc_exch_mgr *em;
struct fc_exch_pool *pool;