On Wed, Aug 13, 2014 at 07:21:35PM -0400, Brian Foster wrote:
> On Thu, Aug 14, 2014 at 06:59:29AM +1000, Dave Chinner wrote:
> > On Wed, Aug 13, 2014 at 08:59:32AM -0400, Brian Foster wrote:
> > > Perhaps I'm missing some context... I don't follow how removing the
> > > error check doesn't solve the problem. It clearly closes the race and
> > > perhaps there are other means of doing the same thing, but what part of
> > > the problem does that leave unresolved?
> >
> > Anything that does:
> >
> > xfs_buf_iorequest(bp);
> > if (bp->b_error)
> > xfs_buf_relse(bp);
> >
> > is susceptible to the same race condition. based on bp->b_error
> > being set asynchronously and before the buffer IO completion
> > processing is complete.
> >
>
> Understood, by why would anything do that (as opposed to
> xfs_buf_iowait())? I don't see that we do that anywhere today
> (the check buried within xfs_buf_iowait() notwithstanding of course).
"Why" is not important - the fact is the caller *owns* the buffer
and so the above fragment of code is valid behaviour. If there is
an error on the buffer after xfs_buf_iorequest() request returns on
a synchronous IO, then it's a bug if there is still IO in progress
on that buffer.
We can't run IO completion synchronously from xfs_buf_bio_end_io in
this async dispatch error case - we cannot detect it as any
different from IO completion in interrupt context - and so we need
to have some kind of reference protecting the buffer from being
freed from under the completion.
i.e. the bug is that a synchronous buffer has no active reference
while it is sitting on the completion workqueue - it's references
are owned by other contexts that can drop them without regard to
the completion status of the buffer.
For async IO we transfer a reference and the lock to the IO context,
which gets dropped in xfs_buf_iodone_work when all the IO is
complete. Synchronous IO needs this protection, too.
As a proof of concept, adding this to the start of
xfs_buf_iorequest():
+ /*
+ * synchronous IO needs it's own reference count. async IO
+ * inherits the submitter's reference count.
+ */
+ if (!(bp->b_flags & XBF_ASYNC))
+ xfs_buf_hold(bp);
And this to the synchronous IO completion case for
xfs_buf_iodone_work():
else {
ASSERT(read && bp->b_ops);
complete(&bp->b_iowait);
+ xfs_buf_rele(bp);
}
Should ensure that all IO carries a reference count and the buffer
cannot be freed until all IO processing has been completed.
This means it does not matter what the buffer owner does after
xfs_buf_iorequest() - even unconditionally calling xfs_buf_relse()
will not result in use-after-free as the b_hold count will not go to
zero until the IO completion processing has been finalised.
Fixing the rest of the mess (i.e. determining how to deal with
submission/completion races) is going to require more effort and
thought. For the moment, though, correctly reference counting
buffers will solve the use-after-free without changing any
other behaviour.
> From what I can see, all it really guarantees is that the submission has
> either passed/failed the write verifier, yes?
No. It can also mean it wasn't rejected by the lower layersi as
they process the bio passed by submit_bio(). e.g. ENODEV, because
the underlying device has been hot-unplugged, EIO because the
buffer is beyond the end of the device, etc.
> > > It looks like submit_bio() manages this by providing the error through
> > > the callback (always). It also doesn't look like submission path is
> > > guaranteed to be synchronous either (consider md, which appears to use
> > > workqueues and kernel threads)), so I'm not sure that '...;
> > > xfs_buf_iorequest(bp); if (bp->b_error)' is really safe anywhere unless
> > > you're explicitly looking for a write verifier error or something and
> > > do nothing further on the buf contingent on completion (e.g., freeing it
> > > or something it depends on).
> >
> > My point remains that it *should be safe*, and the intent is that
> > the caller should be able to check for submission errors without
> > being exposed to a use after free situation. That's the bug we need
> > to fix, not say "you can't check for submission errors on
> > synchronous IO" to avoid the race condition.....
> >
>
> Well, technically you can check for submission errors on sync I/O, just
> use the code you posted above. :) What we can't currently do is find out
> when the I/O subsystem is done with the buffer.
By definition, a buffer marked with an error after submission
processing is complete. It should not need to be waited on, and
there-in lies the bug.
> Perhaps the point here is around the semantics of xfs_buf_iowait(). With
> a mechanism that is fundamentally async, the sync variant obviously
> becomes the async mechanism + some kind of synchronization. I'd expect
> that synchronization to not necessarily just tell me whether an error
> occurred, but also tell me when the I/O subsystem is done with the
> object I've passed (e.g., so I'm free to chuck it, scribble over it, put
> it back where I got it, whatever).
>
> My impression is that's the purpose of the b_iowait mechanism.
> Otherwise, what's the point of the whole
> bio_end_io->buf_ioend->b_iodone->buf_ioend round trip dance?
Yes, that's exactly what xfs_buf_iorequest/xfs_buf_iowait() provides
and the b_error indication is an integral part of that
synchronisation mechanism. Unfortunately, that is also the part of
the mechanism that is racy and causing problems.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
