bdrv_replace_child() calls bdrv_check_perm() with error_abort on
loosening permissions. However file-locking operations may fail even
in this case, for example on NFS. And this leads to Qemu crash.

Let's ignore such errors, as we do already on permission update commit
and abort.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

v2: - simplify expression [Eric]
    - fix bug s/new_perm/new_shared

 block/file-posix.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index db4cccbe51..736b4851e3 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -815,6 +815,20 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
 
     switch (op) {
     case RAW_PL_PREPARE:
+        if ((s->perm | new_perm) == s->perm &&
+            (s->shared_perm & new_shared) == s->shared_perm)
+        {
+            /*
+             * We are going to unlock bytes, it should not fail. If fail,
+             * just report it and ignore, like we do for ABORT and COMMIT
+             * anyway.
+             */
+            ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, &local_err);
+            if (local_err) {
+                error_report_err(local_err);
+            }
+            return 0;
+        }
         ret = raw_apply_lock_bytes(s, s->fd, s->perm | new_perm,
                                    ~s->shared_perm | ~new_shared,
                                    false, errp);
-- 
2.18.0

On 3/28/19 2:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_replace_child() calls bdrv_check_perm() with error_abort on
> loosening permissions. However file-locking operations may fail even
> in this case, for example on NFS. And this leads to Qemu crash.
> 
> Let's ignore such errors, as we do already on permission update commit
> and abort.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> v2: - simplify expression [Eric]
>     - fix bug s/new_perm/new_shared
> 
>  block/file-posix.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Am 28.03.2019 um 08:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
> bdrv_replace_child() calls bdrv_check_perm() with error_abort on
> loosening permissions. However file-locking operations may fail even
> in this case, for example on NFS. And this leads to Qemu crash.
> 
> Let's ignore such errors, as we do already on permission update commit
> and abort.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

I think this would better be fixed in block.c code so that unlock never
fails for any block driver.

But we're late for 4.0, so if fixing it in block.c proves difficult,
this might still be better than nothing and I could accept it as a
preliminary solution.

Kevin

28.03.2019 21:40, Kevin Wolf wrote:
> Am 28.03.2019 um 08:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> bdrv_replace_child() calls bdrv_check_perm() with error_abort on
>> loosening permissions. However file-locking operations may fail even
>> in this case, for example on NFS. And this leads to Qemu crash.
>>
>> Let's ignore such errors, as we do already on permission update commit
>> and abort.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> I think this would better be fixed in block.c code so that unlock never
> fails for any block driver.

Hmm. We now only have one .bdrv_check_perm handler - raw_check_perm. And
in this particular case, yes, the only thing we can do is ignoring error
and do not fail on loosening permissions..

If we have more drivers with this callback, what should be the common
behavior?

Do you propose to ignore .bdrv_check_perm errors in common case?

Isn't it better to require, that .bdrv_check_perm handler do not fail on
loosening permissions, and abort if it fail in this case, like it actually
works after this patch?

> 
> But we're late for 4.0, so if fixing it in block.c proves difficult,
> this might still be better than nothing and I could accept it as a
> preliminary solution.
> 
> Kevin
> 


-- 
Best regards,
Vladimir

Am 29.03.2019 um 10:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 28.03.2019 21:40, Kevin Wolf wrote:
> > Am 28.03.2019 um 08:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> bdrv_replace_child() calls bdrv_check_perm() with error_abort on
> >> loosening permissions. However file-locking operations may fail even
> >> in this case, for example on NFS. And this leads to Qemu crash.
> >>
> >> Let's ignore such errors, as we do already on permission update commit
> >> and abort.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > 
> > I think this would better be fixed in block.c code so that unlock never
> > fails for any block driver.
> 
> Hmm. We now only have one .bdrv_check_perm handler - raw_check_perm. And
> in this particular case, yes, the only thing we can do is ignoring error
> and do not fail on loosening permissions..
> 
> If we have more drivers with this callback, what should be the common
> behavior?
> 
> Do you propose to ignore .bdrv_check_perm errors in common case?
> 
> Isn't it better to require, that .bdrv_check_perm handler do not fail on
> loosening permissions, and abort if it fail in this case, like it actually
> works after this patch?

Maybe an assertion in the common code is actually better, yes.

I do think that the common behaviour we want is to ignore
.bdrv_check_perm errors for unlock, but it might be surprising for
drivers that .bdrv_set_perm is called when .bdrv_check_perm failed. So
we need the drivers to be aware of the problem anyway.

Back to your patch: Why do you need to call raw_check_lock_bytes() in
the unlock case? We don't acquire any new permissions and hold the locks
for everything, so nobody else should have taken a conflicting lock.

Kevin

29.03.2019 13:12, Kevin Wolf wrote:
> Am 29.03.2019 um 10:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 28.03.2019 21:40, Kevin Wolf wrote:
>>> Am 28.03.2019 um 08:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> bdrv_replace_child() calls bdrv_check_perm() with error_abort on
>>>> loosening permissions. However file-locking operations may fail even
>>>> in this case, for example on NFS. And this leads to Qemu crash.
>>>>
>>>> Let's ignore such errors, as we do already on permission update commit
>>>> and abort.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> I think this would better be fixed in block.c code so that unlock never
>>> fails for any block driver.
>>
>> Hmm. We now only have one .bdrv_check_perm handler - raw_check_perm. And
>> in this particular case, yes, the only thing we can do is ignoring error
>> and do not fail on loosening permissions..
>>
>> If we have more drivers with this callback, what should be the common
>> behavior?
>>
>> Do you propose to ignore .bdrv_check_perm errors in common case?
>>
>> Isn't it better to require, that .bdrv_check_perm handler do not fail on
>> loosening permissions, and abort if it fail in this case, like it actually
>> works after this patch?
> 
> Maybe an assertion in the common code is actually better, yes.
> 
> I do think that the common behaviour we want is to ignore
> .bdrv_check_perm errors for unlock, but it might be surprising for
> drivers that .bdrv_set_perm is called when .bdrv_check_perm failed. So
> we need the drivers to be aware of the problem anyway.
> 
> Back to your patch: Why do you need to call raw_check_lock_bytes() in
> the unlock case? We don't acquire any new permissions and hold the locks
> for everything, so nobody else should have taken a conflicting lock.
> 

Hmm.. it check not same arguments, so I didn't drop it as raw_apply_lock_bytes.

On the other hand, the only meaning of this raw_check_lock_bytes, is that we'll
print error if it come (when it should not).

Seems OK for me to drop it too and just return 0 immediately.


-- 
Best regards,
Vladimir

29.03.2019 13:55, Vladimir Sementsov-Ogievskiy wrote:
> 29.03.2019 13:12, Kevin Wolf wrote:
>> Am 29.03.2019 um 10:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> 28.03.2019 21:40, Kevin Wolf wrote:
>>>> Am 28.03.2019 um 08:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> bdrv_replace_child() calls bdrv_check_perm() with error_abort on
>>>>> loosening permissions. However file-locking operations may fail even
>>>>> in this case, for example on NFS. And this leads to Qemu crash.
>>>>>
>>>>> Let's ignore such errors, as we do already on permission update commit
>>>>> and abort.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>
>>>> I think this would better be fixed in block.c code so that unlock never
>>>> fails for any block driver.
>>>
>>> Hmm. We now only have one .bdrv_check_perm handler - raw_check_perm. And
>>> in this particular case, yes, the only thing we can do is ignoring error
>>> and do not fail on loosening permissions..
>>>
>>> If we have more drivers with this callback, what should be the common
>>> behavior?
>>>
>>> Do you propose to ignore .bdrv_check_perm errors in common case?
>>>
>>> Isn't it better to require, that .bdrv_check_perm handler do not fail on
>>> loosening permissions, and abort if it fail in this case, like it actually
>>> works after this patch?
>>
>> Maybe an assertion in the common code is actually better, yes.
>>
>> I do think that the common behaviour we want is to ignore
>> .bdrv_check_perm errors for unlock, but it might be surprising for
>> drivers that .bdrv_set_perm is called when .bdrv_check_perm failed. So
>> we need the drivers to be aware of the problem anyway.
>>
>> Back to your patch: Why do you need to call raw_check_lock_bytes() in
>> the unlock case? We don't acquire any new permissions and hold the locks
>> for everything, so nobody else should have taken a conflicting lock.
>>
> 
> Hmm.. it check not same arguments, so I didn't drop it as raw_apply_lock_bytes.
> 
> On the other hand, the only meaning of this raw_check_lock_bytes, is that we'll
> print error if it come (when it should not).
> 
> Seems OK for me to drop it too and just return 0 immediately.
> 
> 

sent corresponding v3, renamed a bit:
[PATCH v3] block/file-posix: do not fail on unlock bytes


-- 
Best regards,
Vladimir

Am 29.03.2019 um 11:55 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.03.2019 13:12, Kevin Wolf wrote:
> > Am 29.03.2019 um 10:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> 28.03.2019 21:40, Kevin Wolf wrote:
> >>> Am 28.03.2019 um 08:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>>> bdrv_replace_child() calls bdrv_check_perm() with error_abort on
> >>>> loosening permissions. However file-locking operations may fail even
> >>>> in this case, for example on NFS. And this leads to Qemu crash.
> >>>>
> >>>> Let's ignore such errors, as we do already on permission update commit
> >>>> and abort.
> >>>>
> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>>
> >>> I think this would better be fixed in block.c code so that unlock never
> >>> fails for any block driver.
> >>
> >> Hmm. We now only have one .bdrv_check_perm handler - raw_check_perm. And
> >> in this particular case, yes, the only thing we can do is ignoring error
> >> and do not fail on loosening permissions..
> >>
> >> If we have more drivers with this callback, what should be the common
> >> behavior?
> >>
> >> Do you propose to ignore .bdrv_check_perm errors in common case?
> >>
> >> Isn't it better to require, that .bdrv_check_perm handler do not fail on
> >> loosening permissions, and abort if it fail in this case, like it actually
> >> works after this patch?
> > 
> > Maybe an assertion in the common code is actually better, yes.
> > 
> > I do think that the common behaviour we want is to ignore
> > .bdrv_check_perm errors for unlock, but it might be surprising for
> > drivers that .bdrv_set_perm is called when .bdrv_check_perm failed. So
> > we need the drivers to be aware of the problem anyway.
> > 
> > Back to your patch: Why do you need to call raw_check_lock_bytes() in
> > the unlock case? We don't acquire any new permissions and hold the locks
> > for everything, so nobody else should have taken a conflicting lock.
> > 
> 
> Hmm.. it check not same arguments, so I didn't drop it as
> raw_apply_lock_bytes.

Not exactly the same, but a subset of the old ones.

> On the other hand, the only meaning of this raw_check_lock_bytes, is
> that we'll print error if it come (when it should not).
> 
> Seems OK for me to drop it too and just return 0 immediately.

Okay, so the plan for v3 is to drop the raw_check_lock_bytes() call
here, and add an assertion in block.c, right?

Kevin

29.03.2019 14:08, Kevin Wolf wrote:
> Am 29.03.2019 um 11:55 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.03.2019 13:12, Kevin Wolf wrote:
>>> Am 29.03.2019 um 10:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 28.03.2019 21:40, Kevin Wolf wrote:
>>>>> Am 28.03.2019 um 08:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>> bdrv_replace_child() calls bdrv_check_perm() with error_abort on
>>>>>> loosening permissions. However file-locking operations may fail even
>>>>>> in this case, for example on NFS. And this leads to Qemu crash.
>>>>>>
>>>>>> Let's ignore such errors, as we do already on permission update commit
>>>>>> and abort.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>
>>>>> I think this would better be fixed in block.c code so that unlock never
>>>>> fails for any block driver.
>>>>
>>>> Hmm. We now only have one .bdrv_check_perm handler - raw_check_perm. And
>>>> in this particular case, yes, the only thing we can do is ignoring error
>>>> and do not fail on loosening permissions..
>>>>
>>>> If we have more drivers with this callback, what should be the common
>>>> behavior?
>>>>
>>>> Do you propose to ignore .bdrv_check_perm errors in common case?
>>>>
>>>> Isn't it better to require, that .bdrv_check_perm handler do not fail on
>>>> loosening permissions, and abort if it fail in this case, like it actually
>>>> works after this patch?
>>>
>>> Maybe an assertion in the common code is actually better, yes.
>>>
>>> I do think that the common behaviour we want is to ignore
>>> .bdrv_check_perm errors for unlock, but it might be surprising for
>>> drivers that .bdrv_set_perm is called when .bdrv_check_perm failed. So
>>> we need the drivers to be aware of the problem anyway.
>>>
>>> Back to your patch: Why do you need to call raw_check_lock_bytes() in
>>> the unlock case? We don't acquire any new permissions and hold the locks
>>> for everything, so nobody else should have taken a conflicting lock.
>>>
>>
>> Hmm.. it check not same arguments, so I didn't drop it as
>> raw_apply_lock_bytes.
> 
> Not exactly the same, but a subset of the old ones.
> 
>> On the other hand, the only meaning of this raw_check_lock_bytes, is
>> that we'll print error if it come (when it should not).
>>
>> Seems OK for me to drop it too and just return 0 immediately.
> 
> Okay, so the plan for v3 is to drop the raw_check_lock_bytes() call
> here, and add an assertion in block.c, right?
> 

Okay, for v4)


-- 
Best regards,
Vladimir

29.03.2019 14:08, Kevin Wolf wrote:
> Am 29.03.2019 um 11:55 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.03.2019 13:12, Kevin Wolf wrote:
>>> Am 29.03.2019 um 10:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 28.03.2019 21:40, Kevin Wolf wrote:
>>>>> Am 28.03.2019 um 08:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>> bdrv_replace_child() calls bdrv_check_perm() with error_abort on
>>>>>> loosening permissions. However file-locking operations may fail even
>>>>>> in this case, for example on NFS. And this leads to Qemu crash.
>>>>>>
>>>>>> Let's ignore such errors, as we do already on permission update commit
>>>>>> and abort.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>
>>>>> I think this would better be fixed in block.c code so that unlock never
>>>>> fails for any block driver.
>>>>
>>>> Hmm. We now only have one .bdrv_check_perm handler - raw_check_perm. And
>>>> in this particular case, yes, the only thing we can do is ignoring error
>>>> and do not fail on loosening permissions..
>>>>
>>>> If we have more drivers with this callback, what should be the common
>>>> behavior?
>>>>
>>>> Do you propose to ignore .bdrv_check_perm errors in common case?
>>>>
>>>> Isn't it better to require, that .bdrv_check_perm handler do not fail on
>>>> loosening permissions, and abort if it fail in this case, like it actually
>>>> works after this patch?
>>>
>>> Maybe an assertion in the common code is actually better, yes.
>>>
>>> I do think that the common behaviour we want is to ignore
>>> .bdrv_check_perm errors for unlock, but it might be surprising for
>>> drivers that .bdrv_set_perm is called when .bdrv_check_perm failed. So
>>> we need the drivers to be aware of the problem anyway.
>>>
>>> Back to your patch: Why do you need to call raw_check_lock_bytes() in
>>> the unlock case? We don't acquire any new permissions and hold the locks
>>> for everything, so nobody else should have taken a conflicting lock.
>>>
>>
>> Hmm.. it check not same arguments, so I didn't drop it as
>> raw_apply_lock_bytes.
> 
> Not exactly the same, but a subset of the old ones.
> 
>> On the other hand, the only meaning of this raw_check_lock_bytes, is
>> that we'll print error if it come (when it should not).
>>
>> Seems OK for me to drop it too and just return 0 immediately.
> 
> Okay, so the plan for v3 is to drop the raw_check_lock_bytes() call
> here, and add an assertion in block.c, right?

but where to assert? No I think - nowhere. We'll abort on error_abort anyway.

And what to assert? We can't do such check in common bdrv_check_perm,
as we don't keep previous cumulative permissions..

So I think, my v3 is OK.

-- 
Best regards,
Vladimir