Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1572235 > unrolled thread

Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to allocate more pages per call

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-02 11:00 +0100
    Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Christoph Hellwig <hch@infradead.org> - 2017-02-02 12:00 +0100
      Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-02 12:20 +0100
        Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Jeff Layton <jlayton@redhat.com> - 2017-02-02 14:10 +0100
          Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-03 08:40 +0100
            Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Linus Torvalds <torvalds@linux-foundation.org> - 2017-02-03 19:30 +0100
              Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-03 20:10 +0100
                Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Linus Torvalds <torvalds@linux-foundation.org> - 2017-02-03 20:30 +0100
                  Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Steve Capper <steve.capper@linaro.org> - 2017-02-13 11:00 +0100
          Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Christoph Hellwig <hch@infradead.org> - 2017-02-03 08:50 +0100
            Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-03 10:00 +0100
              Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Christoph Hellwig <hch@infradead.org> - 2017-02-03 12:10 +0100
    Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Jan Kara <jack@suse.cz> - 2017-02-02 15:50 +0100
      Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-02 19:30 +0100
        Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Jan Kara <jack@suse.cz> - 2017-02-03 15:50 +0100
    Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-04 04:10 +0100
      Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-04 20:30 +0100
        Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-04 23:20 +0100
      Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-04 23:20 +0100
        Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-05 03:20 +0100
          Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-05 21:20 +0100
            Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-05 22:10 +0100
              Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-05 22:20 +0100
                Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-05 23:10 +0100
                  Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-06 04:10 +0100
                    Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-06 10:10 +0100
                      Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-06 11:00 +0100
                        Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-06 15:20 +0100
                          Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-07 08:20 +0100
                            Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-07 12:40 +0100
                              Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-08 07:00 +0100
                                Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-08 12:20 +0100
                  Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-06 09:40 +0100
          Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-05 22:00 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1574234

On Sun, Feb 5, 2017 at 2:51 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sat, Feb 04, 2017 at 11:11:27PM +0100, Miklos Szeredi wrote:
>
>> Well, it's not historical; at least not yet.  The deadlock is there
>> alright: mmap fuse file to addr; read byte from mapped page -> page
>> locked; this triggeres read request served in same process but
>> separate thread; write addr-headerlen to fuse dev; trying to lock same
>> page -> deadlock.
>
> Let me see if I got it straight - you have the same fuse file mmapped
> in two processes, one of them being fuse server (either sharing the
> entire address space, or the same area mapped in both).  Another process
> faults the sucker in; filemap_fault() locks the page and goes
> fuse_readpage() -> fuse_do_readpage() -> fuse_send_read() ->
> -> fuse_request_send() -> __fuse_request_send() which puts request into
> queue and goes to sleep in request_wait_answer().  Eventually, read()
> on /dev/fuse (or splice(), whatever) by server picks that request and reply
> is formed and fed back into /dev/fuse.  There we (in fuse_do_dev_write())
> call copy_out_args(), which tries to copy into our (still locked) page
> a piece of data coming from server-supplied iovec.  As it is, you
> are calling get_user_pages_fast(), triggering handle_mm_fault().  Since that
> malicous FPOS of a server tried to feed you the _same_ mmapped file, you
> hit a deadlock.  In server's context.  Correct?

Yes.

> Convoluted, but possible.  But.  Why the hell do we care whether that deadlock
> hits in get_user_pages_fast() or in copy_from_user()?  Put it another way,
> what difference does it make whether we take that fault with or without
> FR_LOCKED in req->flags?

The difference is that if the page fault happens without FR_LOCKED,
then we can abort the request then and there (done by moving the
request to to_end1 and calling request_end() on it).

If abort happens with FR_LOCKED, we can't end the request now, because
data is possibly being copied to/from the request args.  But we are
guaranteed that the request will end shortly, because no sleeping
under FR_LOCKED is allowed.

But with copy_from_user() page fault and copy aren't separated and so
we don't know whether it's safe to abort or not.

Maybe I'm missing something obvious, but I don't see a simple way out of this.

>> The deadlock can be broken by aborting or force unmounting: return
>> error for original read request; page unlocked; device write can get
>> page lock and return.
>>
>> The reason we need to prohibit pagefault while copying is that when
>> request is aborted and the caller returns the memory in the request
>> may become invalid (e.g. data from stack).
>
> ???
>
> IDGI.  Your request is marked aborted and should presumably fail, so
> that when request_wait_answer() wakes up and finds it screwed, fuse_readpage()
> would just return an error and filemap_fault() will return VM_FAULT_SIGBUS,
> with page left not uptodate and _not_ inserted into page tables.  What's
> leaking where?

That case is fine.   But nothing guarantees that fuse_abort_conn()
won't be called (in the non-deadlock case) when data is being copied
to the request args.  Ending the request at such a point could easily
lead to use after free,

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1574284

On Sun, Feb 05, 2017 at 09:15:24PM +0100, Miklos Szeredi wrote:

> That case is fine.   But nothing guarantees that fuse_abort_conn()
> won't be called (in the non-deadlock case) when data is being copied
> to the request args.  Ending the request at such a point could easily
> lead to use after free,

So why not leave ending it to your fuse_dev_do_write()/fuse_dev_do_read()?
See the reply I'd just sent (your mail arrived while I'd been writing that
one - saw it only after I'd sent mine).

Basically, what if we keep FR_LOCKED through *all* fuse_dev_do_{read,write}(),
rather than dropping and regaining it many times and have fuse_abort_conn()
skip request_end() on FR_LOCKED ones?

[toc] | [prev] | [next] | [standalone]

#1574285

On Sun, Feb 5, 2017 at 10:01 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Feb 05, 2017 at 09:15:24PM +0100, Miklos Szeredi wrote:
>
>> That case is fine.   But nothing guarantees that fuse_abort_conn()
>> won't be called (in the non-deadlock case) when data is being copied
>> to the request args.  Ending the request at such a point could easily
>> lead to use after free,
>
> So why not leave ending it to your fuse_dev_do_write()/fuse_dev_do_read()?
> See the reply I'd just sent (your mail arrived while I'd been writing that
> one - saw it only after I'd sent mine).
>
> Basically, what if we keep FR_LOCKED through *all* fuse_dev_do_{read,write}(),
> rather than dropping and regaining it many times and have fuse_abort_conn()
> skip request_end() on FR_LOCKED ones?

Then we can't break out of that deadlock: we wait until
fuse_dev_do_write() is done until calling request_end() which
ultimately results in unlocking page.  But fuse_dev_do_write() won't
complete until the page is unlocked.

The only way out that I see is to have a refcount on all pages in
args.  Which means copying everything not already in refcountable page
(i.e. args on stack) to a page array.   It's definitely doable, but
needs time to sort out, and I'm definitely lacking that (overlayfs
currently trumps fuse).

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1574289

On Sun, Feb 05, 2017 at 10:19:20PM +0100, Miklos Szeredi wrote:

> Then we can't break out of that deadlock: we wait until
> fuse_dev_do_write() is done until calling request_end() which
> ultimately results in unlocking page.  But fuse_dev_do_write() won't
> complete until the page is unlocked.

Wait a sec.  What happens if

process A: fuse_lookup()
		struct fuse_entry_out outarg on stack
		...
		fuse_request_send() with req->out.args[0].value = &outarg
			sleep in request_wait_answer() on req->waitq
server: read the request, write reply
	fuse_dev_do_write()
		copy_out_args()
			fuse_copy_args()
				fuse_copy_one()
					FR_LOCKED is guaranteed to be set
					fuse_copy_do()
process C on another CPU: umount -f
	fuse_conn_abort()
		end_requests()
			request_end()
				set FR_FINISHED
				wake A up (via req->waitq)
process A: regain CPU
	bugger off from request_wait_answer(), through __fuse_request_send(),
	fuse_request_send(), fuse_simple_request(), fuse_lookup_name(),
	fuse_lookup() and out of fuse_lookup().

In the meanwhile, server in fuse_copy_do() does memcpy() to what used to
be outarg, corrupting the stack of process A.

Sure, you need to hit a fairly narrow window, especially if you are to
cause damage in A, but AFAICS it's not impossible.  Consider e.g. the
situation when you lose CPU on preempt on the way to memcpy(); in that
case server might come back when A has incremented its stack footprint
again.  Or A might end up taking a hardware interrupt and handling it
on the normal kernel stack, etc.

Looks like *any* scenario where fuse_conn_abort() manages to run during
that memcpy() has potential for that kind of trouble; any SMP box appears
to be vulnerable, along with preempt UP...

Am I missing something that prevents that kind of problem?

> The only way out that I see is to have a refcount on all pages in
> args.  Which means copying everything not already in refcountable page
> (i.e. args on stack) to a page array.   It's definitely doable, but
> needs time to sort out, and I'm definitely lacking that (overlayfs
> currently trumps fuse).

Hrm...  Then maybe I'll have to try and cook something along those lines;
AFAICS the current mainline is vulnerable...

[toc] | [prev] | [next] | [standalone]

#1574338

On Sun, Feb 05, 2017 at 10:04:45PM +0000, Al Viro wrote:

> Sure, you need to hit a fairly narrow window, especially if you are to
> cause damage in A, but AFAICS it's not impossible.  Consider e.g. the
> situation when you lose CPU on preempt on the way to memcpy(); in that
> case server might come back when A has incremented its stack footprint
> again.  Or A might end up taking a hardware interrupt and handling it
> on the normal kernel stack, etc.
> 
> Looks like *any* scenario where fuse_conn_abort() manages to run during
> that memcpy() has potential for that kind of trouble; any SMP box appears
> to be vulnerable, along with preempt UP...
> 
> Am I missing something that prevents that kind of problem?

For that matter, it doesn't have to be on-stack - e.g. fuse_get_link()
has kmalloc'ed buffer for destination, kfree'd upon failure.  Have the
damn thing lose the timeslice in fuse_copy_do() and you might very well
end up spraying user-supplied data over whatever ends up picking your
kfreed buffer.  That one could be reasonably dealt with if we switched
to page_alloc() and stuffed it into the ->pages[] instead...

Some observations regarding the arguments:
	* stack footprint is atrocious.  Consider e.g. fuse_mknod() - you
get 16 bytes of fuse_mknod_in + 120 bytes of struct fuse_args + 128 bytes
of fuse_entry_out.  All on stack, and that's on top of whatever the
callchain already has eaten, which might include e.g. nfsd stuff or
ecryptfs, etc.  Or fuse_get_parent(), for that matter, with 128 bytes of
fuse_entry_out + 120 bytes of fuse_args, both on stack.  This one is
guaranteed to have a nasty call chain - fuse_get_parent() <- reconnect_one()
<- reconnect_path() <- exportfs_decode_fh() (itself with a 256-byte array of
char on stack) <- nfsd_set_fh_dentry() <- fh_verify() <- a bunch of call
chains in nfsd.
	* "out" args (i.e. reply) are probably best dealt with by having
coallocated with request itself - some already are and the sizes tend
to be fixed and not too large (->get_link() is an exception, and it's
probably better handled as mentioned above).
	* "in" args (request) are in some cases easily dealt with by
coallocating with request, but there's a large class of situations where
we are passing dentry->d_name.name and then there's fuse_symlink().
The last one is ugly - potentially up to a page worth of data, coming
straight from method caller; usually it's a part of getname() result,
but e.g. ecryptfs might have it kmalloc'ed, nfsd - picked from sunrpc
request payload, etc.

	AFAICS, your argument applies to the requests that have
some page(s) locked until the request completion (unlock_page() either
by ->end() callback or in the originator of request).  If so, I would
rather mark those as "call request_end() early"; they seem to have
the non-page parts of args hosted in req->misc, so for them it's not
a problem.

	So how about this:

* explicit FR_END_IMMEDIATELY on read/write-related requests
* no FR_LOCKED flipping in lock_request()/unlock_request()
* modifying the call of end_requests() in fuse_abort_conn() so that it
would skip request_end() for everything that isn't marked FR_END_IMMEDIATELY
* make fuse_copy_pages() grab page references around the actual
fuse_copy_page() - grab req->waitq.lock, check FR_ABORTED, grab a page
reference in case it's not, drop req->waitq.lock and bugger off if FR_ABORTED
was set.  Adjust fuse_try_move_page() accordingly.

Do you see any problems with that approach for minimal fix?  If all requests
in need of FR_END_IMMEDIATELY turn out to have non-page part of args already
embedded into req->misc, it looks like this ought to suffice.  I probably
could post something along those lines tomorrow, if you see any serious
problems with that - please yell...

[toc] | [prev] | [next] | [standalone]

#1574481

On Mon, Feb 6, 2017 at 4:05 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:

> Some observations regarding the arguments:
>         * stack footprint is atrocious.  Consider e.g. fuse_mknod() - you
> get 16 bytes of fuse_mknod_in + 120 bytes of struct fuse_args + 128 bytes
> of fuse_entry_out.  All on stack, and that's on top of whatever the
> callchain already has eaten, which might include e.g. nfsd stuff or
> ecryptfs, etc.  Or fuse_get_parent(), for that matter, with 128 bytes of
> fuse_entry_out + 120 bytes of fuse_args, both on stack.  This one is
> guaranteed to have a nasty call chain - fuse_get_parent() <- reconnect_one()
> <- reconnect_path() <- exportfs_decode_fh() (itself with a 256-byte array of
> char on stack) <- nfsd_set_fh_dentry() <- fh_verify() <- a bunch of call
> chains in nfsd.

Indeed.

>         * "out" args (i.e. reply) are probably best dealt with by having
> coallocated with request itself - some already are and the sizes tend
> to be fixed and not too large (->get_link() is an exception, and it's
> probably better handled as mentioned above).
>         * "in" args (request) are in some cases easily dealt with by
> coallocating with request, but there's a large class of situations where
> we are passing dentry->d_name.name and then there's fuse_symlink().
> The last one is ugly - potentially up to a page worth of data, coming
> straight from method caller; usually it's a part of getname() result,
> but e.g. ecryptfs might have it kmalloc'ed, nfsd - picked from sunrpc
> request payload, etc.
>
>         AFAICS, your argument applies to the requests that have
> some page(s) locked until the request completion (unlock_page() either
> by ->end() callback or in the originator of request).  If so, I would
> rather mark those as "call request_end() early"; they seem to have
> the non-page parts of args hosted in req->misc, so for them it's not
> a problem.

Yes, I think only page lock can be used to deadlock inside
fuse_dev_read/write().  So requests that don't have locked pages
should be okay  with just waiting until copy_to/from_user() finishes
and only then proceeding with the abort.

Those that have locked pages must be able to be aborted during
copy_to/from_user() because the copy itself may try to acquire the
page lock.

So yes, if we want to switch to copy_to/from_user(), then we can just
fix the page refcounting for read and write requests and handle the
two cases differently.

>         So how about this:
>
> * explicit FR_END_IMMEDIATELY on read/write-related requests
> * no FR_LOCKED flipping in lock_request()/unlock_request()
> * modifying the call of end_requests() in fuse_abort_conn() so that it
> would skip request_end() for everything that isn't marked FR_END_IMMEDIATELY
> * make fuse_copy_pages() grab page references around the actual
> fuse_copy_page() - grab req->waitq.lock, check FR_ABORTED, grab a page
> reference in case it's not, drop req->waitq.lock and bugger off if FR_ABORTED
> was set.  Adjust fuse_try_move_page() accordingly.
>
> Do you see any problems with that approach for minimal fix?  If all requests
> in need of FR_END_IMMEDIATELY turn out to have non-page part of args already
> embedded into req->misc, it looks like this ought to suffice.  I probably
> could post something along those lines tomorrow, if you see any serious
> problems with that - please yell...

See previous mail, I don't think there's an issue with the current
code.  Other than being convoluted as hell.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1574509

On Mon, Feb 06, 2017 at 10:08:06AM +0100, Miklos Szeredi wrote:

> Yes, I think only page lock can be used to deadlock inside
> fuse_dev_read/write().  So requests that don't have locked pages
> should be okay  with just waiting until copy_to/from_user() finishes
> and only then proceeding with the abort.

Actually, looking at that some more, this might be not true.  Anything
that takes ->mmap_sem exclusive and *not* killable makes for another
source of deadlock.

Initial page fault takes ->mmap_sem shared.  OK, request sent to
server and server tries to read() it.  In the meanwhile, something
has closed userfaultfd for the same mm_struct.  We have userfaultfd_release()
block on attempt to take ->mmap_sem exclusive and from now on any attempt
to grab ->mmap_sem shared will deadlock.  And get_user_pages(), as well
as copy_to_user(), etc. can end up doing just that.  It doesn't have to
be an mmap of the same file, BTW - any page fault would do.

All you really need is to have server sharing address space with the
process that steps into original page fault, plus an evicted page
of any nature (anon mmap, whatever) being used as a destination of
read() in server.

down_read() inside down_read() is fine, unless there had been down_write()
in between.  And there are unkillable down_write() on ->mmap_sem -
userfaultfd_release() being one example of such.  Many of those can and
probably should become down_write_killable(), but this one can't - there
might be nothing to deliver the signal to, if the final close() happens
e.g. from exit(2).

Warning: the above might be completely bogus - I'm on way too large
uptime at the moment and most of the last day had been spent digging
through various convoluted code, so take the above with a cartload of
salt.  _If_ it's true, that kind of deadlock won't be possible to
break with killing anything or doing umount -f, though.

> 
> Those that have locked pages must be able to be aborted during
> copy_to/from_user() because the copy itself may try to acquire the
> page lock.
> 
> So yes, if we want to switch to copy_to/from_user(), then we can just
> fix the page refcounting for read and write requests and handle the
> two cases differently.
> 
> >         So how about this:
> >
> > * explicit FR_END_IMMEDIATELY on read/write-related requests
> > * no FR_LOCKED flipping in lock_request()/unlock_request()
> > * modifying the call of end_requests() in fuse_abort_conn() so that it
> > would skip request_end() for everything that isn't marked FR_END_IMMEDIATELY
> > * make fuse_copy_pages() grab page references around the actual
> > fuse_copy_page() - grab req->waitq.lock, check FR_ABORTED, grab a page
> > reference in case it's not, drop req->waitq.lock and bugger off if FR_ABORTED
> > was set.  Adjust fuse_try_move_page() accordingly.
> >
> > Do you see any problems with that approach for minimal fix?  If all requests
> > in need of FR_END_IMMEDIATELY turn out to have non-page part of args already
> > embedded into req->misc, it looks like this ought to suffice.  I probably
> > could post something along those lines tomorrow, if you see any serious
> > problems with that - please yell...
> 
> See previous mail, I don't think there's an issue with the current
> code.  Other than being convoluted as hell.

OK - I'm an idiot and I've managed to misread fuse_abort_conn() despite having
reread it many times last couple of days.  And yes, state transitions of
requests are convoluted as hell ;-/

Anyway, bedtime for me.  With any luck the scare above re ->mmap_sem *is*
bogus and I'll find "Al, you are an idiot - deadlock on ->mmap_sem can't
happen for <reasons>" from somebody in the mailbox when I get up...

[toc] | [prev] | [next] | [standalone]

#1574815

On Mon, Feb 6, 2017 at 10:57 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Feb 06, 2017 at 10:08:06AM +0100, Miklos Szeredi wrote:
>
>> Yes, I think only page lock can be used to deadlock inside
>> fuse_dev_read/write().  So requests that don't have locked pages
>> should be okay  with just waiting until copy_to/from_user() finishes
>> and only then proceeding with the abort.
>
> Actually, looking at that some more, this might be not true.  Anything
> that takes ->mmap_sem exclusive and *not* killable makes for another
> source of deadlock.
>
> Initial page fault takes ->mmap_sem shared.  OK, request sent to
> server and server tries to read() it.  In the meanwhile, something
> has closed userfaultfd for the same mm_struct.  We have userfaultfd_release()
> block on attempt to take ->mmap_sem exclusive and from now on any attempt
> to grab ->mmap_sem shared will deadlock.  And get_user_pages(), as well
> as copy_to_user(), etc. can end up doing just that.  It doesn't have to
> be an mmap of the same file, BTW - any page fault would do.
>
> All you really need is to have server sharing address space with the
> process that steps into original page fault, plus an evicted page
> of any nature (anon mmap, whatever) being used as a destination of
> read() in server.
>
> down_read() inside down_read() is fine, unless there had been down_write()
> in between.  And there are unkillable down_write() on ->mmap_sem -
> userfaultfd_release() being one example of such.  Many of those can and
> probably should become down_write_killable(), but this one can't - there
> might be nothing to deliver the signal to, if the final close() happens
> e.g. from exit(2).
>
> Warning: the above might be completely bogus - I'm on way too large
> uptime at the moment and most of the last day had been spent digging
> through various convoluted code, so take the above with a cartload of
> salt.  _If_ it's true, that kind of deadlock won't be possible to
> break with killing anything or doing umount -f, though.

It's not bogus, the deadlock is there.

But I think it's breakable in the same way: if the deadlocked request
is aborted, the fault will release the page lock as well as mmap_sem,
and from there things will resolve themselves.

But you are definitely right about needing to clean up that mess in
fuse/dev.c and doing so by fixing up the arg refcounting for just the
read and write requests is going to be a lot simpler than having to do
that for all of them (which was my original plan).

So, I'll have a go at that sometime.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1575424

On Mon, Feb 06, 2017 at 03:18:42PM +0100, Miklos Szeredi wrote:

> But I think it's breakable in the same way: if the deadlocked request
> is aborted, the fault will release the page lock as well as mmap_sem,
> and from there things will resolve themselves.

Right you are - original holder of ->mmap_sem is waiting for request and
abort will wake it up...

> But you are definitely right about needing to clean up that mess in
> fuse/dev.c and doing so by fixing up the arg refcounting for just the
> read and write requests is going to be a lot simpler than having to do
> that for all of them (which was my original plan).

Speaking of refcounting - what's going on with fuse_file one?  My reading
of that code is that you have 4 states of that thing:
	* new (just created, fallback request allocated, use fuse_file_free()
to kill).  Refcount is 0.
	* intermediate - in fact it's already opened, but still not
put into ->private_data.  Refcount is still 0.  Use fuse_sync_release() to kill.
	* live - normal refcounting (fuse_file_get()/fuse_file_put()).
	* shutdown - refcount has reached 0.  Can't happen until ->release()
(obviously - ->private_data holds a counting reference), some pieces of
fuse_sync_release() correspond to some stuff in fuse_release_common(), some -
to final fuse_file_put().

To make it even more convoluted, cuse is using fuse_sync_release() and
apparently relies upon no references being grabbed after fuse_do_open(),
so that thing can be called with refcount 0 *or* refcount 1.

Another thing: what guarantees that places in writepages-related paths
where we store a reference into req->ff won't hit a request with already
non-NULL ->ff?

[toc] | [prev] | [next] | [standalone]

#1575579

On Tue, Feb 07, 2017 at 07:19:09AM +0000, Al Viro wrote:

> Speaking of refcounting - what's going on with fuse_file one?  My reading
> of that code is that you have 4 states of that thing:
> 	* new (just created, fallback request allocated, use fuse_file_free()
> to kill).  Refcount is 0.
> 	* intermediate - in fact it's already opened, but still not
> put into ->private_data.  Refcount is still 0.  Use fuse_sync_release() to kill.
> 	* live - normal refcounting (fuse_file_get()/fuse_file_put()).
> 	* shutdown - refcount has reached 0.  Can't happen until ->release()
> (obviously - ->private_data holds a counting reference), some pieces of
> fuse_sync_release() correspond to some stuff in fuse_release_common(), some -
> to final fuse_file_put().
> 
> To make it even more convoluted, cuse is using fuse_sync_release() and
> apparently relies upon no references being grabbed after fuse_do_open(),
> so that thing can be called with refcount 0 *or* refcount 1.

Indeed, ugly.  Following should clean that up somewhat.  The states remain what
they are but refcounting is at least consistent now.  This also fixes a missing
FR_FORCE in the sync case.
 
> Another thing: what guarantees that places in writepages-related paths
> where we store a reference into req->ff won't hit a request with already
> non-NULL ->ff?

Well, it is set before being sent (queued onto queued_writes or queued on the
fuse device), but not when queued as secondary request onto an already in-flight
one.  It looks okay to me.

Thanks,
Miklos

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 811fd8929a18..e816166ce42f 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -473,7 +473,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	if (err) {
 		fuse_sync_release(ff, flags);
 	} else {
-		file->private_data = fuse_file_get(ff);
+		file->private_data = ff;
 		fuse_finish_open(inode, file);
 	}
 	return err;
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 2401c5dabb2a..94cc4ab01f9a 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -58,7 +58,7 @@ struct fuse_file *fuse_file_alloc(struct fuse_conn *fc)
 	}
 
 	INIT_LIST_HEAD(&ff->write_entry);
-	atomic_set(&ff->count, 0);
+	atomic_set(&ff->count, 1);
 	RB_CLEAR_NODE(&ff->polled_node);
 	init_waitqueue_head(&ff->poll_wait);
 
@@ -75,7 +75,7 @@ void fuse_file_free(struct fuse_file *ff)
 	kfree(ff);
 }
 
-struct fuse_file *fuse_file_get(struct fuse_file *ff)
+static struct fuse_file *fuse_file_get(struct fuse_file *ff)
 {
 	atomic_inc(&ff->count);
 	return ff;
@@ -100,6 +100,7 @@ static void fuse_file_put(struct fuse_file *ff, bool sync)
 			iput(req->misc.release.inode);
 			fuse_put_request(ff->fc, req);
 		} else if (sync) {
+			__set_bit(FR_FORCE, &req->flags);
 			__clear_bit(FR_BACKGROUND, &req->flags);
 			fuse_request_send(ff->fc, req);
 			iput(req->misc.release.inode);
@@ -146,7 +147,7 @@ int fuse_do_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
 		ff->open_flags &= ~FOPEN_DIRECT_IO;
 
 	ff->nodeid = nodeid;
-	file->private_data = fuse_file_get(ff);
+	file->private_data = ff;
 
 	return 0;
 }
@@ -297,13 +298,9 @@ static int fuse_release(struct inode *inode, struct file *file)
 
 void fuse_sync_release(struct fuse_file *ff, int flags)
 {
-	WARN_ON(atomic_read(&ff->count) > 1);
+	WARN_ON(atomic_read(&ff->count) != 1);
 	fuse_prepare_release(ff, flags, FUSE_RELEASE);
-	__set_bit(FR_FORCE, &ff->reserved_req->flags);
-	__clear_bit(FR_BACKGROUND, &ff->reserved_req->flags);
-	fuse_request_send(ff->fc, ff->reserved_req);
-	fuse_put_request(ff->fc, ff->reserved_req);
-	kfree(ff);
+	fuse_file_put(ff, true);
 }
 EXPORT_SYMBOL_GPL(fuse_sync_release);
 
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 91307940c8ac..83f797271aef 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -732,7 +732,6 @@ void fuse_read_fill(struct fuse_req *req, struct file *file,
 int fuse_open_common(struct inode *inode, struct file *file, bool isdir);
 
 struct fuse_file *fuse_file_alloc(struct fuse_conn *fc);
-struct fuse_file *fuse_file_get(struct fuse_file *ff);
 void fuse_file_free(struct fuse_file *ff);
 void fuse_finish_open(struct inode *inode, struct file *file);
 

[toc] | [prev] | [next] | [standalone]

#1576287

On Tue, Feb 07, 2017 at 12:35:54PM +0100, Miklos Szeredi wrote:
> > Another thing: what guarantees that places in writepages-related paths
> > where we store a reference into req->ff won't hit a request with already
> > non-NULL ->ff?
> 
> Well, it is set before being sent (queued onto queued_writes or queued on the
> fuse device), but not when queued as secondary request onto an already in-flight
> one.  It looks okay to me.

>  void fuse_sync_release(struct fuse_file *ff, int flags)
>  {
> -	WARN_ON(atomic_read(&ff->count) > 1);
> +	WARN_ON(atomic_read(&ff->count) != 1);
>  	fuse_prepare_release(ff, flags, FUSE_RELEASE);
> -	__set_bit(FR_FORCE, &ff->reserved_req->flags);
> -	__clear_bit(FR_BACKGROUND, &ff->reserved_req->flags);
> -	fuse_request_send(ff->fc, ff->reserved_req);
> -	fuse_put_request(ff->fc, ff->reserved_req);
> -	kfree(ff);
> +	fuse_file_put(ff, true);

Umm...  At the very least, that deserves a comment re "iput(NULL) is a no-op
and since the refcount is 1 and everything's synchronous, we are fine with
not doing igrab/iput here".  There's enough mysteries in that code as it is...

	Speaking of mysteries - how can ->private_data ever be NULL in
fuse_release_common()?  AFAICS, it's only called from ->release() instances
and those are only called after ->open() or ->atomic_open() on that struct file
has returned 0.  On the ->open() side, it means fuse_do_open() having returned
0; on ->atomic_open() one - fuse_create_open() having done the same.  Neither
is possible with ->private_data remaining NULL, and I don't see any places
that would modify it afterwards...

	Another thing: am I right assuming that ff->nodeid will be the same
for all ff over given inode (== get_node_id(inode))?  What about ff->fh?
Is that a per-open thing, or will it be identical for all opens of the same
inode?

[toc] | [prev] | [next] | [standalone]

#1576468

On Wed, Feb 8, 2017 at 6:54 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Feb 07, 2017 at 12:35:54PM +0100, Miklos Szeredi wrote:
>> > Another thing: what guarantees that places in writepages-related paths
>> > where we store a reference into req->ff won't hit a request with already
>> > non-NULL ->ff?
>>
>> Well, it is set before being sent (queued onto queued_writes or queued on the
>> fuse device), but not when queued as secondary request onto an already in-flight
>> one.  It looks okay to me.
>
>>  void fuse_sync_release(struct fuse_file *ff, int flags)
>>  {
>> -     WARN_ON(atomic_read(&ff->count) > 1);
>> +     WARN_ON(atomic_read(&ff->count) != 1);
>>       fuse_prepare_release(ff, flags, FUSE_RELEASE);
>> -     __set_bit(FR_FORCE, &ff->reserved_req->flags);
>> -     __clear_bit(FR_BACKGROUND, &ff->reserved_req->flags);
>> -     fuse_request_send(ff->fc, ff->reserved_req);
>> -     fuse_put_request(ff->fc, ff->reserved_req);
>> -     kfree(ff);
>> +     fuse_file_put(ff, true);
>
> Umm...  At the very least, that deserves a comment re "iput(NULL) is a no-op
> and since the refcount is 1 and everything's synchronous, we are fine with
> not doing igrab/iput here".  There's enough mysteries in that code as it is...

Added comment.

>         Speaking of mysteries - how can ->private_data ever be NULL in
> fuse_release_common()?  AFAICS, it's only called from ->release() instances
> and those are only called after ->open() or ->atomic_open() on that struct file
> has returned 0.  On the ->open() side, it means fuse_do_open() having returned
> 0; on ->atomic_open() one - fuse_create_open() having done the same.  Neither
> is possible with ->private_data remaining NULL, and I don't see any places
> that would modify it afterwards...

Goes back to v2.6.15 (commit fd72faac95d7 "[PATCH] FUSE: atomic
create+open").  Didn't make sense back then, and it doesn't now.
Fixed.

>         Another thing: am I right assuming that ff->nodeid will be the same
> for all ff over given inode (== get_node_id(inode))?

Yes.  Except for cuse, where it's zero.

>  What about ff->fh?
> Is that a per-open thing, or will it be identical for all opens of the same
> inode?

A per-open thing (opaque identifier used by userspace fs to identify
the open file)

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1574453

On Sun, Feb 5, 2017 at 11:04 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Feb 05, 2017 at 10:19:20PM +0100, Miklos Szeredi wrote:
>
>> Then we can't break out of that deadlock: we wait until
>> fuse_dev_do_write() is done until calling request_end() which
>> ultimately results in unlocking page.  But fuse_dev_do_write() won't
>> complete until the page is unlocked.
>
> Wait a sec.  What happens if
>
> process A: fuse_lookup()
>                 struct fuse_entry_out outarg on stack
>                 ...
>                 fuse_request_send() with req->out.args[0].value = &outarg
>                         sleep in request_wait_answer() on req->waitq
> server: read the request, write reply
>         fuse_dev_do_write()
>                 copy_out_args()
>                         fuse_copy_args()
>                                 fuse_copy_one()
>                                         FR_LOCKED is guaranteed to be set
>                                         fuse_copy_do()
> process C on another CPU: umount -f
>         fuse_conn_abort()
>                 end_requests()
>                         request_end()
>                                 set FR_FINISHED
>                                 wake A up (via req->waitq)
> process A: regain CPU
>         bugger off from request_wait_answer(), through __fuse_request_send(),
>         fuse_request_send(), fuse_simple_request(), fuse_lookup_name(),
>         fuse_lookup() and out of fuse_lookup().
>
> In the meanwhile, server in fuse_copy_do() does memcpy() to what used to
> be outarg, corrupting the stack of process A.
>
> Sure, you need to hit a fairly narrow window, especially if you are to
> cause damage in A, but AFAICS it's not impossible.  Consider e.g. the
> situation when you lose CPU on preempt on the way to memcpy(); in that
> case server might come back when A has incremented its stack footprint
> again.  Or A might end up taking a hardware interrupt and handling it
> on the normal kernel stack, etc.
>
> Looks like *any* scenario where fuse_conn_abort() manages to run during
> that memcpy() has potential for that kind of trouble; any SMP box appears
> to be vulnerable, along with preempt UP...
>
> Am I missing something that prevents that kind of problem?

Yes: if FR_LOCKED is set, then we leave the request alone in
fuse_abort_conn().  Then, when the copy is finished and
request_unlock() is called, we return -ENOENT to fuse_dev_do_write(),
which in turn calls request_end() to wake up the original caller,
which gets -ECONNABORTED.

So basically FR_LOCKED is protecting the copy, which is guaranteed to
be atomic due to the get_user_pages magic that faults in all pages
beforehand.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1574283

On Sun, Feb 05, 2017 at 01:51:49AM +0000, Al Viro wrote:

> IDGI.  Your request is marked aborted and should presumably fail, so
> that when request_wait_answer() wakes up and finds it screwed, fuse_readpage()
> would just return an error and filemap_fault() will return VM_FAULT_SIGBUS,
> with page left not uptodate and _not_ inserted into page tables.  What's
> leaking where?

Egads...  Do you mean that req->pages[] contents can be dropped by connection
abort right under fuse_copy_pages()?  Or is it that args[...].value can
end up freed under you?

<goes to look at the ->end() callbacks>

Both, apparently, plus the request initiator might have seen that
request has failed and buggered off, with args[...].value pointing to
what used to be initiator's stack frame.  Is that what you are talking about?


If so, why not mark request as "being handled by fuse_dev_do_{read,write}()"
for the duration, and leave the request_end() on such requests for
fuse_dev_do_{read,write}(), seeing that they will call request_end() for such
anyway?

Looks like your FR_LOCKED is not far from that already.  Why not stop
dropping/regaining FR_LOCKED in lock_request()/unlock_request() and simply
have your
                end_requests(fc, &to_end2);
in fuse_abort_conn() skip the actual calls of request_end()?

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web