Q. Switch open_exec() and sys_uselib() to do_open_filp()

Q. Switch open_exec() and sys_uselib() to do_open_filp()

[hooanon05]

By the commit 6e8341a11eb21826b7192d0bb88cb5b44900a9af
"Switch open_exec() and sys_uselib() to do_open_filp()",
FMODE_EXEC flag is passed to struct file. I have no objection nor
question about that.

But it is set to file->f_flags instead of f_mode.
Is this intended behaviour?


J. R. Okajima

Re: Q. Switch open_exec() and sys_uselib() to do_open_filp()

[Al Viro]

On Mon, May 11, 2009 at 09:55:17AM +0900, hooanon05@yahoo.co.jp wrote:
> 
> By the commit 6e8341a11eb21826b7192d0bb88cb5b44900a9af
> "Switch open_exec() and sys_uselib() to do_open_filp()",
> FMODE_EXEC flag is passed to struct file. I have no objection nor
> question about that.
> 
> But it is set to file->f_flags instead of f_mode.
> Is this intended behaviour?

It is previously existing behaviour.  Check what path_lookup_open() does
and how had it used to be called.

And no, very little about intents makes any sense, including that part.
Call graph of the entire lookup-and-open-related machinery least of all.
So we have to untangle that mound of mating tapeworms, one by one ;-/

Re: Q. Switch open_exec() and sys_uselib() to do_open_filp()

[hooanon05]

Al Viro:
> On Mon, May 11, 2009 at 09:55:17AM +0900, hooanon05@yahoo.co.jp wrote:
> > 
> > By the commit 6e8341a11eb21826b7192d0bb88cb5b44900a9af
> > "Switch open_exec() and sys_uselib() to do_open_filp()",
> > FMODE_EXEC flag is passed to struct file. I have no objection nor
> > question about that.
> > 
> > But it is set to file->f_flags instead of f_mode.
> > Is this intended behaviour?
> 
> It is previously existing behaviour.  Check what path_lookup_open() does
> and how had it used to be called.

My poor English may make me misunderstood.
Please let me make sure.
Do you mean FMODE_EXEC was passed to struct file before this commit?

Previous open_exec() used to call
	file = nameidata_to_filp(&nd, O_RDONLY|O_LARGEFILE)
and it calls
	filp = __dentry_open(nd->path.dentry, nd->path.mnt, flags, filp,
				     NULL);
__dentry_open()
{
	f->f_flags = flags;
	f->f_mode = ((flags+1) & O_ACCMODE) | FMODE_LSEEK |
				FMODE_PREAD | FMODE_PWRITE;
	;;;
}

So FMODE_EXEC was not set to f_flags/f_mode, O_RDONLY|O_LARGEFILE only.

After the commit, open_exec() calls
	file = do_filp_open(AT_FDCWD, name,
				O_LARGEFILE | O_RDONLY | FMODE_EXEC, 0,
				MAY_EXEC | MAY_OPEN);
and it calls
	filp = nameidata_to_filp(&nd, open_flag);

So f_flags becomes containing FMODE_EXEC, O_LARGEFILE | O_RDONLY | FMODE_EXEC.
This change of the behaviour is my question since a strict type fmode_t
was defined in 2.6.28 or 29 and FMODE_EXEC looks being set to f_mode.

If setting FMODE_xxx to f_flags is intended, it is OK.


J. R. Okajima

Re: Q. Switch open_exec() and sys_uselib() to do_open_filp()

[Al Viro]

On Mon, May 11, 2009 at 11:46:15AM +0900, hooanon05@yahoo.co.jp wrote:
> My poor English may make me misunderstood.
> Please let me make sure.
> Do you mean FMODE_EXEC was passed to struct file before this commit?
> 
> Previous open_exec() used to call
> 	file = nameidata_to_filp(&nd, O_RDONLY|O_LARGEFILE)
> and it calls
> 	filp = __dentry_open(nd->path.dentry, nd->path.mnt, flags, filp,
> 				     NULL);
> __dentry_open()
> {
> 	f->f_flags = flags;
> 	f->f_mode = ((flags+1) & O_ACCMODE) | FMODE_LSEEK |
> 				FMODE_PREAD | FMODE_PWRITE;
> 	;;;
> }
> 
> So FMODE_EXEC was not set to f_flags/f_mode, O_RDONLY|O_LARGEFILE only.

Nope.  It ended up in nd->intent.open.flags due to path_lookup_open().
Then lookup_instantiate_filp() from a filesystem that might care about
intents did
        nd->intent.open.file = __dentry_open(dget(dentry), mntget(nd->mnt),
                                             nd->intent.open.flags - 1,
                                             nd->intent.open.file,
                                             open);
and nameidata_to_filp() ended up picking nd->intent.open.file.

So f_flags is exactly where that thing used to end up, if it ended up
anywhere at all.

Re: Q. Switch open_exec() and sys_uselib() to do_open_filp()

[hooanon05]

Al Viro:
> Nope.  It ended up in nd->intent.open.flags due to path_lookup_open().

Yes.


> Then lookup_instantiate_filp() from a filesystem that might care about
> intents did
>         nd->intent.open.file = __dentry_open(dget(dentry), mntget(nd->mnt),
>                                              nd->intent.open.flags - 1,
>                                              nd->intent.open.file,
>                                              open);
> and nameidata_to_filp() ended up picking nd->intent.open.file.

FS supporting lookup_instantiate_filp() is rare, isn't it?
Simple grep told me it is called by fuse, nfsv4, cifs, and 9p only.
In other FS, FMODE_EXEC was dropped since nameidata_to_filp() (from
open_exec() directly) doesn't pick intent.open.flags up.

Anyway, I could confirm that setting FMODE_EXEC to f_flags is intended.
Thank you for your quick responces.


J. R. Okajima

Re: Q. Switch open_exec() and sys_uselib() to do_open_filp()

[Al Viro]

On Mon, May 11, 2009 at 02:20:09PM +0900, hooanon05@yahoo.co.jp wrote:
> 
> Al Viro:
> > Nope.  It ended up in nd->intent.open.flags due to path_lookup_open().
> 
> Yes.
> 
> 
> > Then lookup_instantiate_filp() from a filesystem that might care about
> > intents did
> >         nd->intent.open.file = __dentry_open(dget(dentry), mntget(nd->mnt),
> >                                              nd->intent.open.flags - 1,
> >                                              nd->intent.open.file,
> >                                              open);
> > and nameidata_to_filp() ended up picking nd->intent.open.file.
> 
> FS supporting lookup_instantiate_filp() is rare, isn't it?
> Simple grep told me it is called by fuse, nfsv4, cifs, and 9p only.
> In other FS, FMODE_EXEC was dropped since nameidata_to_filp() (from
> open_exec() directly) doesn't pick intent.open.flags up.

The rest of filesystems simply ignore FMODE_EXEC completely, no matter where
it is.
 
> Anyway, I could confirm that setting FMODE_EXEC to f_flags is intended.
> Thank you for your quick responces.

NOTE: "intended" != "is promised to stay that way".  Indeed, this thread is
an excellent demonstration of the reasons for massaging the entire nightmare
into saner shape.  The call graph in the current form is an obscenity, the
number of possible codepaths is way too high, thanks to the intents mess and
the paths taken by the data are interesting and hard to trace, to put it
very mildly.

The things *will* change, and final destination of FMODE_EXEC may very well
be among those.  The commit in question did the following:
	* eliminate two special codepaths
	* make FMODE_EXEC treatment consistent (it's always propagated to
f_flags) without functionality changes (filesystems that ignored the intents
are ignoring that bit in f_flags completely anyway).

We might end up changing the treatment of that thing and it might very well
end up in f_mode.  However, that would be a separate patch and it's not for
this point in cycle.