On Sat, Dec 26, 2015 at 9:28 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> On Sat, Dec 26, 2015 at 08:26:13PM -0500, Trond Myklebust wrote:
>
>> The may_open() is now happening before NFS gets a chance to issue the
>> OPEN rpc call, which is a change w.r.t. the lookup intent code. The
>> ordering is significant because it means the OPEN can no longer prime
>> the access cache.
>
> Always had... Consider e.g. device nodes; there ->open() might very well
> have side effects, and ones you do not want to allow for any random caller.
> Permissions checks always had been done prior to ->open(), it's not something
> new.
When we did lookup intents, the d_revalidate() would take a lookup
intent, which would trigger the OPEN call for NFS before we got
anywhere near permissions checking. As I said, the removal of lookup
intents makes that impossible, and hence now we have the inefficiency.
>
>> >> > Merry Christmas
>> >>
>> >> Suggestions Al?
>> >
>> > Make nfs_permission() relax the checks when it sees MAY_OPEN, if you know
>> > that things will be caught by server anyway?
>>
>> That can work as long as we're guaranteed that everything that calls
>> inode_permission() with MAY_OPEN on a regular file will also follow up
>> with a vfs_open() or dentry_open() on success. Is this always the
>> case?
>
> 1) in do_tmpfile(), followed by do_dentry_open() (not reachable by NFS since
> it doesn't have ->tmpfile() instance anyway)
>
> 2) in atomic_open(), after the call of ->atomic_open() has succeeded.
Right, so we don't care there either.
> 3) in do_last(), followed on success by vfs_open()
>
> That's all. All calls of inode_permission() that get MAY_OPEN come from
> may_open(), and there's no other callers of that puppy.
OK.
> PS: I'm not sure we want to carry that MAY_OPEN in op->acc_mode, actually.
> may_open() gets acc_mode without MAY_OPEN only when called from do_last()
> in case of O_PATH open. The check in the very beginning of may_open()
> triggers only in that case and might as well be replaced with
> if (likely(!(open_flag & O_PATH))) {
> error = may_open(&nd->path, acc_mode, open_flag);
> if (error)
> goto out;
> }
> in that call site (one right after finish_open_created:). Then we could
> bloody well have may_open() do
> error = inode_permission(inode, acc_mode | MAY_OPEN);
> and forget about MAY_OPEN in op->acc_mode.
>
> Something like the patch below should be an equivalent transformation and with
> that it's really clear what's going on with MAY_OPEN. Warning: completely
> untested.
That looks right and would indeed make it easier to trace. I'll push
the changes to nfs_permission()
Thanks!
Trond
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
