[07/12] vfs: do get_write_access() on upper layer of overlayfs

Commit Message

Amir Goldstein Oct. 19, 2016, 7:41 a.m. UTC

On Tue, Oct 18, 2016 at 10:53 PM, Miklos Szeredi <[e-mail]> wrote:
> On Tue, Oct 18, 2016 at 9:41 PM, Amir Goldstein <[e-mail]> wrote:
>> On Fri, Sep 16, 2016 at 3:19 PM, Miklos Szeredi <[e-mail]> wrote:
>>> The problem with writecount is: we want consistent handling of it for
>>> underlying filesystems as well as overlayfs.  Making sure i_writecount is
>>> correct on all layers is difficult.  Instead this patch makes sure that
>>> when write access is acquired, it's always done on the underlying writable
>>> layer (called the upper layer).  We must also make sure to look at the
>>> writecount on this layer when checking for conflicting leases.
>>>
>>> Open for write already updates the upper layer's writecount.  Leaving only
>>> truncate.
>>>
>>> For truncate copy up must happen before get_write_access() so that the
>>> writecount is updated on the upper layer.  Problem with this is if
>>> something fails after that, then copy-up was done needlessly.  E.g. if
>>> break_lease() was interrupted.  Probably not a big deal in practice.
>>>
>>> Another interesting case is if there's a denywrite on a lower file that is
>>> then opened for write or truncated.  With this patch these will succeed,
>>> which is somewhat counterintuitive.  But I think it's still acceptable,
>>> considering that the copy-up does actually create a different file, so the
>>> old, denywrite mapping won't be touched.
>>
>> Miklos,
>> I think this breaks xfstest overlay/013 on v4.8, because execve() does
>> deny write on lower inode and then truncate happens on upper inode.
>
> It does break the xfstest, but as explained in the patch it shouldn't
> be a problem in practice.  I think the test should just be fixed.
>

Right. and so I did fix the test, which had a bug anyway and was not
testing exec+truncate from upper at all.

But I noticed a strange behavior:
When you run t_truncate_self from upper you get success for not being
able to truncate.
When you run t_truncate_self from lower you get an error for being
able to truncate
This is ok as you write, but...
When you re-run t_truncate_self file that just got copied-up you get
segmentation fault
and that is not ok.

You get try the patch to xfstest below to reproduce the problem:

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Miklos Szeredi Oct. 19, 2016, 8:12 a.m. UTC | #1

On Wed, Oct 19, 2016 at 9:41 AM, Amir Goldstein <[e-mail]> wrote:

> But I noticed a strange behavior:
> When you run t_truncate_self from upper you get success for not being
> able to truncate.
> When you run t_truncate_self from lower you get an error for being
> able to truncate
> This is ok as you write, but...
> When you re-run t_truncate_self file that just got copied-up you get
> segmentation fault
> and that is not ok.
>
> You get try the patch to xfstest below to reproduce the problem:

Why would a program want to self truncate itself, except to test denywrite?

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Amir Goldstein Oct. 19, 2016, 8:30 a.m. UTC | #2

On Wed, Oct 19, 2016 at 11:12 AM, Miklos Szeredi <[e-mail]> wrote:
> On Wed, Oct 19, 2016 at 9:41 AM, Amir Goldstein <[e-mail]> wrote:
>
>> But I noticed a strange behavior:
>> When you run t_truncate_self from upper you get success for not being
>> able to truncate.
>> When you run t_truncate_self from lower you get an error for being
>> able to truncate
>> This is ok as you write, but...
>> When you re-run t_truncate_self file that just got copied-up you get
>> segmentation fault
>> and that is not ok.
>>
>> You get try the patch to xfstest below to reproduce the problem:
>
> Why would a program want to self truncate itself, except to test denywrite?
>


Forget it. my re-run test was silly. Because the truncate succeeded
it was trying to run a corrupted executable.

I will send patches to fix the test according to new behavior.

Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Amir Goldstein Oct. 19, 2016, 8:56 a.m. UTC | #3

On Wed, Oct 19, 2016 at 11:30 AM, Amir Goldstein <[e-mail]> wrote:
> On Wed, Oct 19, 2016 at 11:12 AM, Miklos Szeredi <[e-mail]> wrote:
>> On Wed, Oct 19, 2016 at 9:41 AM, Amir Goldstein <[e-mail]> wrote:
>>
>>> But I noticed a strange behavior:
>>> When you run t_truncate_self from upper you get success for not being
>>> able to truncate.
>>> When you run t_truncate_self from lower you get an error for being
>>> able to truncate
>>> This is ok as you write, but...
>>> When you re-run t_truncate_self file that just got copied-up you get
>>> segmentation fault
>>> and that is not ok.
>>>
>>> You get try the patch to xfstest below to reproduce the problem:
>>
>> Why would a program want to self truncate itself, except to test denywrite?
>>
>
>
> Forget it. my re-run test was silly. Because the truncate succeeded
> it was trying to run a corrupted executable.
>
> I will send patches to fix the test according to new behavior.

Sent patches to fstests. Please review.

>
> Thanks,
> Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/tests/overlay/013 b/tests/overlay/013
index 09f3eb1..cd3d0c0 100755
--- a/tests/overlay/013
+++ b/tests/overlay/013
@@ -59,7 +59,7 @@  upperdir=$SCRATCH_DEV/$OVERLAY_UPPER_DIR
 mkdir -p $lowerdir
 mkdir -p $upperdir
 cp $here/src/t_truncate_self $lowerdir/test_lower
-cp $here/src/t_truncate_self $lowerdir/test_upper
+cp $here/src/t_truncate_self $upperdir/test_upper

 _scratch_mount

@@ -67,8 +67,9 @@  _scratch_mount
 # test programs truncate themselfs, all should fail with ETXTBSY
 $SCRATCH_MNT/test_lower
 $SCRATCH_MNT/test_upper
+# run test again after copy-up
+$SCRATCH_MNT/test_lower

 # success, all done
-echo "Silence is golden"
 status=0
 exit
diff --git a/tests/overlay/013.out b/tests/overlay/013.out
index 3e66423..a8ee7cd 100644
--- a/tests/overlay/013.out
+++ b/tests/overlay/013.out
@@ -1,2 +1,2 @@ 
 QA output created by 013
-Silence is golden
+truncate(argv[0]) should have failed