[lkml]   [2017]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 0/5] fs, xfs refcount conversions
On Thu, Apr 20, 2017 at 3:04 PM, Darrick J. Wong
<> wrote:
> 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.

I haven't studied this code, but I think it'd likely be worth keeping
it just to catch any future bug that might appear. If it doesn't
create a problem, we should use refcount_t for anything that is being
used as a reference counter.

> 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.

We _absolutely_ don't want to compile out these checks: the point is
to catch flaws that are discovered and could be exploited in the wild
before updates could be delivered to a system (if they ever are at

> 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.

If it doesn't get in your way, and it really is reference counting
(which it sounds like it is), we should be using refcount_t. The kinds
of bugs we've seen over the last decade continually prove that we'll
see refcount issues where we least expect it, and we need to catch



Kees Cook
Pixel Security

 \ /
  Last update: 2017-04-21 00:19    [W:0.043 / U:0.648 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site