Re: [PATCH v5 05/31] block-backend: special comments for blk_set/get_perm due to fuse

[Hanna Reitz]

On 15.12.21 11:13, Emanuele Giuseppe Esposito wrote:
> On 15/12/2021 09:57, Emanuele Giuseppe Esposito wrote:
> > On 10/12/2021 15:38, Hanna Reitz wrote:
> > > On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote:
> > > > Fuse logic can be classified as I/O, so there is no BQL held
> > > > during its execution. And yet, it uses blk_{get/set}_perm
> > > > functions, that are classified as BQL and clearly require
> > > > the BQL lock. Since there is no easy solution for this,
> > > > add a couple of TODOs and FIXME in the relevant sections of the
> > > > code.
> > >
> > > Well, the problem goes deeper, because we still consider them GS 
> > > functions.  So it’s fine for the permission function 
> > > raw_handle_perm_lock() to call bdrv_get_flags(), which is a GS 
> > > function, and then you still get:
> > >
> > > qemu-storage-daemon: ../block.c:6195: bdrv_get_flags: Assertion 
> > > `qemu_in_main_thread()' failed.
>
> Wait... Now that I read it more carefully I am confused about this. I 
> don't think the above has to do with fuse, right?

It does, because the problem is that the FUSE export code 
(fuse_do_truncate()) calls a permission function (blk_set_perm()), and 
then we assume in those permission functions that they can call GS 
functions, like bdrv_get_flags() (called from raw_handle_perm_lock()).  
So in practice, the permission functions are still effectively GS 
functions, and just dropping the assertions from blk_set/get_perm() 
doesn’t really help.

(This is the state on this patch; patch 7 adds an assertion in 
bdrv_child_try_set_perm(), and so from then on, that assertion fails 
before the one in bdrv_get_flags() can.)

> Can you share the command that makes qemu-storage-daemon fail?

Sure:

$ touch /tmp/fuse-export

$ storage-daemon/qemu-storage-daemon \
  --object iothread,id=iothr0 \
  --blockdev file,node-name=node0,filename=/tmp/fuse-export \
  --export 
fuse,id=exp0,node-name=node0,mountpoint=/tmp/fuse-export,iothread=iothr0,writable=true 
\
  &
[1] 96997

$ truncate /tmp/fuse-export -s 1M
qemu-storage-daemon: ../block.c:6197: bdrv_get_flags: Assertion 
`qemu_in_main_thread()' failed.
truncate: failed to truncate '/tmp/fuse-export' at 1048576 bytes: 
Software caused connection abort
truncate: failed to close '/tmp/fuse-export': Transport endpoint is not 
connected
[1]  + 96997 IOT instruction (core dumped) 
storage-daemon/qemu-storage-daemon --object iothread,id=iothr0 --blockdev

$ fusermount -u /tmp/fuse-export

Relevant part of the stacktrace:

#5  0x00007f68322243a6 in __GI___assert_fail 
(assertion=assertion@entry=0x562380ca2f53 "qemu_in_main_thread()", 
file=file@entry=0x562380ca53cd "../block.c", line=line@entry=6197,
    function=function@entry=0x562380ca78e8 <__PRETTY_FUNCTION__.63> 
"bdrv_get_flags") at assert.c:101
#6  0x0000562380b64283 in bdrv_get_flags (bs=0x562382440680) at 
../block.c:6197
#7  0x0000562380b68506 in bdrv_get_flags (bs=bs@entry=0x562382440680) at 
../block.c:6199
#8  0x0000562380be6d18 in raw_handle_perm_lock (errp=0x7f68277103b0, 
new_shared=31, new_perm=11, op=RAW_PL_PREPARE, bs=0x562382440680) at 
../block/file-posix.c:975
#9  raw_check_perm (bs=0x562382440680, perm=11, shared=31, 
errp=0x7f68277103b0) at ../block/file-posix.c:3172
#10 0x0000562380b66db5 in bdrv_drv_set_perm (errp=0x7f68277103b0, 
tran=0x7f68180038b0, shared_perm=31, perm=11, bs=0x562382440680) at 
../block.c:2272
#11 bdrv_node_refresh_perm (errp=0x7f68277103b0, tran=0x7f68180038b0, 
q=0x0, bs=0x562382440680) at ../block.c:2441
#12 bdrv_list_refresh_perms (list=list@entry=0x56238243a1c0 = {...}, 
q=q@entry=0x0, tran=tran@entry=0x7f68180038b0, 
errp=errp@entry=0x7f68277103b0) at ../block.c:2479
#13 0x0000562380b67098 in bdrv_refresh_perms (bs=0x562382440680, 
errp=errp@entry=0x7f68277103b0) at ../block.c:2542
#14 0x0000562380b6727c in bdrv_child_try_set_perm (c=0x56238243cf70, 
perm=11, shared=31, errp=0x0) at ../block.c:2557
#15 0x0000562380b8632d in blk_set_perm (blk=0x56238243e7a0, perm=11, 
shared_perm=31, errp=errp@entry=0x0) at ../block/block-backend.c:890
#16 0x0000562380b57a7d in fuse_do_truncate 
(exp=exp@entry=0x56238243eb20, size=1048576, 
req_zero_write=req_zero_write@entry=true, 
prealloc=prealloc@entry=PREALLOC_MODE_OFF) at ../block/export/fuse.c:405
#17 0x0000562380b58121 in fuse_setattr (req=0x7f6818003800, inode=1, 
statbuf=0x7f68277104e0, to_set=8, fi=0x7f68277104b0) at 
../block/export/fuse.c:476

> raw_handle_perm_lock() is currently called by these three callbacks:
>     .bdrv_check_perm = raw_check_perm,
>     .bdrv_set_perm   = raw_set_perm,
>     .bdrv_abort_perm_update = raw_abort_perm_update,
>
> all three function pointers are defined as GS functions. So I don't 
> understand why is this failing.

Because this patch explicitly allows I/O code to call 
blk_set/get_perm().  But as you rightly say, all permission functions 
are still classified as GS code, so we can’t allow it.

Hanna

> > > It looks like in this case making bdrv_get_flags() not a GS function 
> > > would be feasible and might solve the problem, but I guess we should 
> > > instead think about adding something like
> > >
> > > if (!exp->growable && !qemu_in_main_thread()) {
> > >      /* Changing permissions like below only works in the main 
> > > thread */
> > >      return -EPERM;
> > > }
> > >
> > > to fuse_do_truncate().
> > >
> > > Ideally, to make up for this, we should probably take the RESIZE 
> > > permission in fuse_export_create() for writable exports in iothreads.
> >
> > I think I got it. I will send the RESIZE permission fix in a separate 
> > patch.
> >
> > Thank you,
> > Emanuele