Message-ID: <1481036059.2573.17.camel@redhat.com>
Subject: Re: FUSE: regression when clearing setuid bits on chown
From: Jeff Layton <jlayton@redhat.com>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>
Date: Tue, 06 Dec 2016 09:54:19 -0500
In-Reply-To: <CAJfpegve7w0YbZy56S1jMrvC2N69JUUvh+W+t=yM3_1eA85nrg@mail.gmail.com>
References: <1480962075.2544.30.camel@redhat.com>
         <20161206100243.GF2622@veci.piliscsaba.szeredi.hu>
         <1481026405.2573.10.camel@redhat.com>
         <20161206143914.GG2622@veci.piliscsaba.szeredi.hu>
         <1481035527.2573.14.camel@redhat.com>
         <CAJfpegve7w0YbZy56S1jMrvC2N69JUUvh+W+t=yM3_1eA85nrg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1531
Lines: 40

On Tue, 2016-12-06 at 15:51 +0100, Miklos Szeredi wrote:
> On Tue, Dec 6, 2016 at 3:45 PM, Jeff Layton <jlayton@redhat.com> wrote:
> 
> > 
> > > 
> > > @@ -1739,8 +1739,6 @@ static int fuse_setattr(struct dentry *e
> > >                * This should be done on write(), truncate() and chown().
> > >                */
> > >               if (!fc->handle_killpriv) {
> > 
> > One more thing too. I don't think we really want to monkey with the mode
> > at all if there is a request to set the mode already in the request. So
> > maybe this should be:
> > 
> >     if (!fc->handle_killpriv && !(attr->ia_mode & ATTR_MODE))
> > 
> > Granted that won't generally happen from normal process context, but we
> > could have knfsd in here too and I think that's possible from there.
> 
> Apparently this can't happen even from knfsd; notify_change() has this comment:
> 
>     /*
>      * We now pass ATTR_KILL_S*ID to the lower level setattr function so
>      * that the function has the ability to reinterpret a mode change
>      * that's due to these bits. This adds an implicit restriction that
>      * no function will ever call notify_change with both ATTR_MODE and
>      * ATTR_KILL_S*ID set.
>      */
>     if ((ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID)) &&
>         (ia_valid & ATTR_MODE))
>         BUG();
> 
> 

Ahh right, I had forgotten about that. Eventually we may want to lift
that restriction, but you can add this to the current patch:

Reviewed-by: Jeff Layton <jlayton@redhat.com>

Thanks for fixing it quickly!