Re: [PATCH 0/5] fs, xfs refcount conversions

[Darrick J. Wong]

On Thu, Apr 20, 2017 at 10:24:04AM -0700, Darrick J. Wong wrote:
> On Thu, Apr 20, 2017 at 08:11:41AM +0000, Reshetova, Elena wrote:
> > 
> >  
> > > v3:
> > >  * fixed header file inclusion
> > 
> > I don't think I have heard anything back on this v3 patch set. 
> > Is there still smth here to fix or could you take the changes in?
> 
> Generally I think it looks ok; it's running through shakedown testing as
> we speak.

Well, it survives the shakedown testing all right, but I can't shake the
feeling that this is overkill.  The xfs_{ef,bu,cu,ru}i_log_item
structures should only ever be referenced by the log itself and the
log-update-done log item: the refcount should only ever go 2 -> 1 -> 0.
We set the refcount to 2 and never increment it.

Given that I've seen occasional refcounting problems with the log intent
items, I think it would suffice to ASSERT in xfs_*i_release that the
refcount isn't already zero.  Using ASSERT is particularly useful
because ASSERT compiles out in release builds.

As for the xlog_ticket, we increment its refcount as part of duplicating
a transaction as a part of committing one transaction (which decrements
the xlog_ticket's refcount) and reusing the log reservation to create a
new transaction.  In this case the refcounting already seems to have
sufficient non-zero checks, and since only one thread can hold a
transaction at any given time, I don't see how overflow protection helps
here.

In other words, I'm ok with better refcount checking but need convincing
that we need to do more than what a simple ASSERT would provide.

--D

> 
> --D
> 
> > 
> > Best Regards,
> > Elena.
> > 
> > > 
> > > Now when new refcount_t type and API are finally merged
> > > (see include/linux/refcount.h), the following
> > > patches convert various refcounters in the xfs susystem from atomic_t
> > > to refcount_t. By doing this we prevent intentional or accidental
> > > underflows or overflows that can led to use-after-free vulnerabilities.
> > > 
> > > 
> > > Elena Reshetova (5):
> > >   fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to
> > >     refcount_t
> > >   fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to
> > >     refcount_t
> > >   fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t
> > >   fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to
> > >     refcount_t
> > >   fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to
> > >     refcount_t
> > > 
> > >  fs/xfs/xfs_bmap_item.c     |  4 ++--
> > >  fs/xfs/xfs_bmap_item.h     |  2 +-
> > >  fs/xfs/xfs_extfree_item.c  |  4 ++--
> > >  fs/xfs/xfs_extfree_item.h  |  2 +-
> > >  fs/xfs/xfs_linux.h         |  1 +
> > >  fs/xfs/xfs_log.c           | 10 +++++-----
> > >  fs/xfs/xfs_log_priv.h      |  2 +-
> > >  fs/xfs/xfs_refcount_item.c |  4 ++--
> > >  fs/xfs/xfs_refcount_item.h |  2 +-
> > >  fs/xfs/xfs_rmap_item.c     |  4 ++--
> > >  fs/xfs/xfs_rmap_item.h     |  2 +-
> > >  11 files changed, 19 insertions(+), 18 deletions(-)
> > > 
> > > --
> > > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html