Re: [PATCH v2 7/7] overlay: test dropping nlink below zero

Amir Goldstein
2017-07-05T11:17:43Z

On Wed, Jul 5, 2017 at 1:09 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Tue, Jul 04, 2017 at 02:40:34PM +0300, Amir Goldstein wrote:
>> nlink of overlay inode could be dropped indefinety by adding
>> un-accounted lower hardlinks underneath a mounted overlay and
>> trying to remove them.
>
> Sorry, I didn't quite follow the hardlink patches, could you please
> describe what is "accounted/un-accounted" hardlinks and the expected
> behavior in commit log? And what does "indefinety" mean? I couldn't find
> it in dictionary.
>

Try the typos dictionary ;) I meant indefinitely

I will try to explain better in change log, but here is the full story:

The simplest way to understand this test is this:
Imagine that you have a tool (e.g. xfs_db) with which
you can add hardlinks, without changing the value of nlink
stored on-disk for the inode. This is exactly what this test does when
it adds lower hardlinks underneath a mounted overlay.

Overlayfs assumes that the lower layer files are not modified
underneath it and if they do, the documentation says:
"Changes to the underlying filesystems while part of a mounted overlay
filesystem are not allowed. If the underlying filesystem is changed,
the behavior of the overlay is undefined, though it will not result in
a crash or deadlock."

As far as I know, this test cannot crash the kernel, but it does trigger
the WARN_ON in drop_link() when nlink drops below zero, so it's not
nice behavior and could possibly results in worse outcomes.

>> +
>> +# Create lower hardlink
>> +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
>> +mkdir -p $lowerdir
>> +touch $lowerdir/0
>> +ln $lowerdir/0 $lowerdir/1
>> +
>> +_scratch_mount
>> +
>> +# Copy up lower hardlink - nlink should be 2

At this time overlay records the initial lower nlink
and assumes it is not going to change.

>> +touch $SCRATCH_MNT/0
>> +
>> +# Add lower hardlinks while overlay is mounted
>
> Why "while overlay is mounted" is needed?

Now overlay *knows* about 2 hardlinks, but we
actually have 4 hardlinks.

>
>> +ln $lowerdir/0 $lowerdir/2
>> +ln $lowerdir/0 $lowerdir/3
>> +
>> +# Unlink un-accounted lowers to drive nlink to 0
>> +rm $SCRATCH_MNT/2
>> +rm $SCRATCH_MNT/3
>
> drive nlink of which file to 0? Sorry, I'm a bit lost on overlay
> hardlink behaviors..

This is new and confusing.
The nlink of the union overlay file is driven to zero, because
it started with nlink 2 (which was copied up from lower) and
then 2 (unaccounted) hardlinks where unlinked dropping nlink
by 2 without ever adding 2.

>
>> +
>> +# Check for getting ENOENT for trying to link !I_LINKABLE with nlink 0
>> +ln $SCRATCH_MNT/0 $SCRATCH_MNT/4
>
> Shouldn't we expect an error message from this ln, if "check for getting
> ENOEND"?

The error is expected if overlay is fooled by this maneuver, but we
detect this use case and fix nlink to > 0 when it is about to drop to zero
because of wrong accounting.

> BTW, this test passed on 4.12-rc7 kernel, I'm not sure if
> that's expected result.
>

Yes, it is expected because in 4.12 hardilnks are always broken on copy up
so they are each of the new lower hardlinks is copied up to a new file
with nlink 1,
so there is no "union nlink accounting" in v4.12 at all and there is
nothing to fool.