linux-kernel - Re: [PATCH 3/3] ovl: redirect on rename-dir

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOQ4uxjJ7Nk1CAxbzQv=gctB_zDP7wx-ac3PMb=OshgujYeUgA@mail.gmail.com>
Date:   Sun, 20 Nov 2016 13:39:35 +0200
From:   Amir Goldstein <amir73il@...il.com>
To:     Miklos Szeredi <miklos@...redi.hu>
Cc:     Raphael Hertzog <hertzog@...ian.org>,
        Konstantin Khlebnikov <koct9i@...il.com>,
        Miklos Szeredi <mszeredi@...hat.com>,
        "linux-unionfs@...r.kernel.org" <linux-unionfs@...r.kernel.org>,
        Guillem Jover <guillem@...ian.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] ovl: redirect on rename-dir

On Fri, Nov 18, 2016 at 5:37 PM, Amir Goldstein <amir73il@...il.com> wrote:
> On Thu, Nov 17, 2016 at 12:00 AM, Miklos Szeredi <miklos@...redi.hu> wrote:
>>
>> On Mon, Nov 14, 2016 at 5:25 PM, Amir Goldstein <amir73il@...il.com> wrote:
>> > On Sun, Nov 13, 2016 at 12:00 PM, Amir Goldstein <amir73il@...il.com> wrote:
>>
>> >> Looks goods, except for the case of change from relative to absolute
>> >> redirect of the victim dentry. IIUC, ovl_set_redirect() will return immediately
>> >> because ovl_dentry_is_redirect() and will not get to setting the absolute
>> >> redirect.
>> >>
>> >
>> > I added some more tests to catch this problem at:
>> > https://github.com/amir73il/unionmount-testsuite.git #ovl_rename_dir
>>
>> Thanks for testing.
>>
>> Force pushed updated version to the usual place:
>>
>>    git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git #redirect
>>
>
> Found one typo and one bug in error that can cause crash on dput(ERR_PTR(err)):
>
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index 21ddac7..0daac51 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -15,7 +15,7 @@ config OVERLAY_FS_REDIRECT_DIR
>         help
>           If this config option is enabled then overlay filesystems will use
>           redirects when renaming directories by default.  In this case it is
> -         still possible possible to turn off redirects globally with the
> +         still possible to turn off redirects globally with the
>           "redirect_dir=off" module option or on a filesystem instance basis
>           with the "redirect_dir=off" mount option.
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 5eaa9f9..a19fc5c 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -105,11 +105,12 @@ static int ovl_lookup_single(struct dentry
> *base, struct ovl_lookup_data *d,
>
>         this = lookup_one_len_unlocked(name, base, namelen);
>         if (IS_ERR(this)) {
> -               if (PTR_ERR(this) == -ENOENT ||
> -                   PTR_ERR(this) == -ENAMETOOLONG) {
> +               err = PTR_ERR(this);
> +               if (err == -ENOENT || err == -ENAMETOOLONG) {
>                         this = NULL;
> +                       goto out;
>                 }
> -               goto out;
> +               return err;
>         }
>         if (!this->d_inode)
>                 goto put_and_out;
>

I just realized that this bug is already in overlayfs-next, so posted
a patch to fix it.

>> This also has the xattr feature thing replaced with mount option,
>> module param and kernel config option.
>>
>
> I like the kernel config/module param/mount option for
> enabling/disabling the feature.
>
> But I still think that we should write the features xattr on the first
> redirect rename.
> The features xattr tell us what can be found on the layer, so we would
> be wise to
> keep it around for all sorts of backward compatibility aspect.
>
> Amir.