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.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/block-backend.c | 10 ++++++++++
 block/export/fuse.c   | 16 ++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 1f0bda578e..7a4b50a8f0 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -888,6 +888,11 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm,
                  Error **errp)
 {
     int ret;
+    /*
+     * FIXME: blk_{get/set}_perm should be always called under
+     * BQL, but it is not the case right now (see block/export/fuse.c)
+     */
+    /* assert(qemu_in_main_thread()); */
 
     if (blk->root && !blk->disable_perm) {
         ret = bdrv_child_try_set_perm(blk->root, perm, shared_perm, errp);
@@ -904,6 +909,11 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm,
 
 void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm)
 {
+    /*
+     * FIXME: blk_{get/set}_perm should be always called under
+     * BQL, but it is not the case right now (see block/export/fuse.c)
+     */
+    /* assert(qemu_in_main_thread()); */
     *perm = blk->perm;
     *shared_perm = blk->shared_perm;
 }
diff --git a/block/export/fuse.c b/block/export/fuse.c
index 823c126d23..7ceb8d783b 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -89,7 +89,10 @@ static int fuse_export_create(BlockExport *blk_exp,
     /* For growable exports, take the RESIZE permission */
     if (args->growable) {
         uint64_t blk_perm, blk_shared_perm;
-
+        /*
+         * FIXME: blk_{get/set}_perm should not be here, as permissions
+         * should be modified only under BQL and here we are not!
+         */
         blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm);
 
         ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE,
@@ -400,6 +403,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size,
 
     /* Growable exports have a permanent RESIZE permission */
     if (!exp->growable) {
+
+        /*
+         * FIXME: blk_{get/set}_perm should not be here, as permissions
+         * should be modified only under BQL and here we are not!
+         */
         blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm);
 
         ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE,
@@ -413,7 +421,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size,
                        truncate_flags, NULL);
 
     if (!exp->growable) {
-        /* Must succeed, because we are only giving up the RESIZE permission */
+        /*
+         * Must succeed, because we are only giving up the RESIZE permission.
+         * FIXME: blk_{get/set}_perm should not be here, as permissions
+         * should be modified only under BQL and here we are not!
+         */
         blk_set_perm(exp->common.blk, blk_perm, blk_shared_perm, &error_abort);
     }
 
-- 
2.27.0

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.

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.

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/block-backend.c | 10 ++++++++++
>   block/export/fuse.c   | 16 ++++++++++++++--
>   2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 1f0bda578e..7a4b50a8f0 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -888,6 +888,11 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm,
>                    Error **errp)
>   {
>       int ret;
> +    /*
> +     * FIXME: blk_{get/set}_perm should be always called under
> +     * BQL, but it is not the case right now (see block/export/fuse.c)
> +     */
> +    /* assert(qemu_in_main_thread()); */
>   
>       if (blk->root && !blk->disable_perm) {
>           ret = bdrv_child_try_set_perm(blk->root, perm, shared_perm, errp);
> @@ -904,6 +909,11 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm,
>   
>   void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm)
>   {
> +    /*
> +     * FIXME: blk_{get/set}_perm should be always called under
> +     * BQL, but it is not the case right now (see block/export/fuse.c)
> +     */
> +    /* assert(qemu_in_main_thread()); */
>       *perm = blk->perm;
>       *shared_perm = blk->shared_perm;
>   }
> diff --git a/block/export/fuse.c b/block/export/fuse.c
> index 823c126d23..7ceb8d783b 100644
> --- a/block/export/fuse.c
> +++ b/block/export/fuse.c
> @@ -89,7 +89,10 @@ static int fuse_export_create(BlockExport *blk_exp,
>       /* For growable exports, take the RESIZE permission */
>       if (args->growable) {
>           uint64_t blk_perm, blk_shared_perm;
> -
> +        /*
> +         * FIXME: blk_{get/set}_perm should not be here, as permissions
> +         * should be modified only under BQL and here we are not!
> +         */

I believe we are, BlockExportDriver.create() is called by blk_exp_add(), 
which looks very much like it runs in the main thread (calling 
bdrv_lookup_bs(), bdrv_try_set_aio_context(), blk_new(), and whatnot).

Hanna

>           blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm);
>   
>           ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE,
> @@ -400,6 +403,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size,
>   
>       /* Growable exports have a permanent RESIZE permission */
>       if (!exp->growable) {
> +
> +        /*
> +         * FIXME: blk_{get/set}_perm should not be here, as permissions
> +         * should be modified only under BQL and here we are not!
> +         */
>           blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm);
>   
>           ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE,
> @@ -413,7 +421,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size,
>                          truncate_flags, NULL);
>   
>       if (!exp->growable) {
> -        /* Must succeed, because we are only giving up the RESIZE permission */
> +        /*
> +         * Must succeed, because we are only giving up the RESIZE permission.
> +         * FIXME: blk_{get/set}_perm should not be here, as permissions
> +         * should be modified only under BQL and here we are not!
> +         */
>           blk_set_perm(exp->common.blk, blk_perm, blk_shared_perm, &error_abort);
>       }
>   

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

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?
Can you share the command that makes qemu-storage-daemon fail?

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.

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

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
>