[PATCH] vfs: fix propagation of atomic_open create error on negative dentry

[PATCH] vfs: fix propagation of atomic_open create error on negative dentry

[Sage Weil]

Hi Miklos,

I found some time to track down the O_CREAT error code issue.  Patch is 
below.  Does this look correct?

I was reproducing with

 ceph-fuse mnt
 mkdir -p mnt/a
 cd mnt
 .../pjd-fstest-20080816/fstest -u 65534 -g 65534 open a/foo O_RDONLY,O_CREAT 0644

EACCES is expected, ENOENT was returned instead.

Thanks!
sage

(BTW: strangely, the correct value was returned if I traverse across 
mounts, e.g.

 mkdir -p mnt/a
 .../pjd-fstest-20080816/fstest -u 65534 -g 65534 open mnt/a/foo O_RDONLY,O_CREAT 0644
)


>From d438bfcae681251c957bcd2b6a2559567faee218 Mon Sep 17 00:00:00 2001
From: Sage Weil <sage@inktank.com>
Date: Wed, 15 Aug 2012 13:30:12 -0700
Subject: [PATCH] vfs: fix propagation of atomic_open create error on negative
 dentry

If ->atomic_open() returns -ENOENT, we take care to return the create
error (e.g., EACCES), if any.  Do the same when ->atomic_open() returns 1
and provides a negative dentry.

This fixes a regression where an unprivileged open O_CREAT fails with
ENOENT instead of EACCES, introduced with the new atomic_open code.  It
is tested by the open/08.t test in the pjd posix test suite, and was
observed on top of fuse (backed by ceph-fuse).

Signed-off-by: Sage Weil <sage@inktank.com>
---
 fs/namei.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 26c28ec..db76b86 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2489,6 +2489,10 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,
 			dput(dentry);
 			dentry = file->f_path.dentry;
 		}
+		if (create_error && dentry->d_inode == NULL) {
+			error = create_error;
+			goto out;
+		}
 		goto looked_up;
 	}
 
-- 
1.7.9

Re: [PATCH] vfs: fix propagation of atomic_open create error on negative dentry

[Miklos Szeredi]

Sage Weil <sage@inktank.com> writes:

> Hi Miklos,
>
> I found some time to track down the O_CREAT error code issue.  Patch is 
> below.  Does this look correct?
>
> I was reproducing with
>
>  ceph-fuse mnt
>  mkdir -p mnt/a
>  cd mnt
>  .../pjd-fstest-20080816/fstest -u 65534 -g 65534 open a/foo O_RDONLY,O_CREAT 0644
>
> EACCES is expected, ENOENT was returned instead.

I don't quite undersdand what is happending here.

Why does ceph-fuse return ENOENT here?  Returning ENOENT for O_CREAT
only makes sense if the parent directory doesn't exist, but that's not
the case here.

Thanks,
Miklos

Re: [PATCH] vfs: fix propagation of atomic_open create error on negative dentry

[Miklos Szeredi]

Miklos Szeredi <miklos@szeredi.hu> writes:

> Sage Weil <sage@inktank.com> writes:
>
>> Hi Miklos,
>>
>> I found some time to track down the O_CREAT error code issue.  Patch is 
>> below.  Does this look correct?
>>
>> I was reproducing with
>>
>>  ceph-fuse mnt
>>  mkdir -p mnt/a
>>  cd mnt
>>  .../pjd-fstest-20080816/fstest -u 65534 -g 65534 open a/foo O_RDONLY,O_CREAT 0644
>>
>> EACCES is expected, ENOENT was returned instead.
>
> I don't quite undersdand what is happending here.
>
> Why does ceph-fuse return ENOENT here?  Returning ENOENT for O_CREAT
> only makes sense if the parent directory doesn't exist, but that's not
> the case here.

More specifically, the initial lookup will return ENOENT, that's fine.

But in that case fuse_atomic_open() will do a FUSE_CREATE for which
ENOENT is not the right answer.

Can you post the relevant part from the filesytem's debug log?

Thanks,
Miklos

Re: [fuse-devel] [PATCH] vfs: fix propagation of atomic_open create error on negative dentry

[Jean-Pierre André]

Hi,

Miklos Szeredi wrote:
> Sage Weil<sage@inktank.com>  writes:
>
>
>> Hi Miklos,
>>
>> I found some time to track down the O_CREAT error code issue.  Patch is
>> below.  Does this look correct?
>>
>> I was reproducing with
>>
>>   ceph-fuse mnt
>>   mkdir -p mnt/a
>>   cd mnt
>>   .../pjd-fstest-20080816/fstest -u 65534 -g 65534 open a/foo O_RDONLY,O_CREAT 0644
>>
>> EACCES is expected, ENOENT was returned instead.
>>
> I don't quite undersdand what is happending here.
>
> Why does ceph-fuse return ENOENT here?  Returning ENOENT for O_CREAT
> only makes sense if the parent directory doesn't exist, but that's not
> the case here.
>

In this test, the parent directory exists, but it is not
writable by the process which tries to create the file.

According to opengroup, from
http://pubs.opengroup.org/onlinepubs/000095399/functions/open.html


    The /open/() function shall fail if:

    [EACCES]
        Search permission is denied on a component of the path prefix,
        or the file exists and the permissions specified by /oflag/ are
        denied, or *the file does not exist and write permission is
        denied for the parent directory of the file to be created*, or
        O_TRUNC is specified and write permission is denied.


Regards

Jean-Pierre

Re: [PATCH] vfs: fix propagation of atomic_open create error on negative dentry

[Sage Weil]

On Thu, 16 Aug 2012, Miklos Szeredi wrote:
> Miklos Szeredi <miklos@szeredi.hu> writes:
> 
> > Sage Weil <sage@inktank.com> writes:
> >
> >> Hi Miklos,
> >>
> >> I found some time to track down the O_CREAT error code issue.  Patch is 
> >> below.  Does this look correct?
> >>
> >> I was reproducing with
> >>
> >>  ceph-fuse mnt
> >>  mkdir -p mnt/a
> >>  cd mnt
> >>  .../pjd-fstest-20080816/fstest -u 65534 -g 65534 open a/foo O_RDONLY,O_CREAT 0644
> >>
> >> EACCES is expected, ENOENT was returned instead.
> >
> > I don't quite undersdand what is happending here.
> >
> > Why does ceph-fuse return ENOENT here?  Returning ENOENT for O_CREAT
> > only makes sense if the parent directory doesn't exist, but that's not
> > the case here.
> 
> More specifically, the initial lookup will return ENOENT, that's fine.
> 
> But in that case fuse_atomic_open() will do a FUSE_CREATE for which
> ENOENT is not the right answer.

I didn't dig deeply into what fuse_lookup() itself was doing, but the 
short version is:

- namei.c's atomic_open() gets -EACCES from the may_o_create() check, sets 
create_error, and clears O_CREAT from the flags

- fuse_atomic_open() calls fuse_lookup(), which returns NULL and leaves 
entry->d_inode == NULL.  ceph-fuse's lookup method returned -ENOENT.  
fuse_lookup() is just hashing the negative dentry instead of returning 
ENOENT.

- fuse_atomic_open() sees O_CREAT is not set and calls finish_no_open().

- the existing ENOENT -> EACCES check in atomic_open() (below) fails 
because fuse_atomic_open() returned 1:

	if (error < 0) {
		if (create_error && error == -ENOENT)
			error = create_error;
		goto out;
	}

This patch just applies the same ENOENT->EACCES logic to the equivalent 
negative/null dentry case.

> Can you post the relevant part from the filesytem's debug log?

The relevant bit is

unique: 11, opcode: GETATTR (3), nodeid: 1, insize: 56, pid: 8502
   unique: 11, success, outsize: 120
unique: 12, opcode: LOOKUP (1), nodeid: 1, insize: 42, pid: 8502
   unique: 12, success, outsize: 144
unique: 13, opcode: GETATTR (3), nodeid: 1099511627776, insize: 56, pid: 8502
   unique: 13, success, outsize: 120
unique: 14, opcode: GETATTR (3), nodeid: 1099511627776, insize: 56, pid: 8502
   unique: 14, success, outsize: 120
unique: 15, opcode: LOOKUP (1), nodeid: 1099511627776, insize: 45, pid: 8502
   unique: 15, error: -2 (No such file or directory), outsize: 16

Thanks-
sage

Re: [PATCH] vfs: fix propagation of atomic_open create error on negative dentry

[Miklos Szeredi]

Sage Weil <sage-4GqslpFJ+cxBDgjK7y7TUQ@public.gmane.org> writes:

> On Thu, 16 Aug 2012, Miklos Szeredi wrote:
>> Miklos Szeredi <miklos-sUDqSbJrdHQHWmgEVkV9KA@public.gmane.org> writes:
>> 
>> > Sage Weil <sage-4GqslpFJ+cxBDgjK7y7TUQ@public.gmane.org> writes:
>> >
>> >> Hi Miklos,
>> >>
>> >> I found some time to track down the O_CREAT error code issue.  Patch is 
>> >> below.  Does this look correct?
>> >>
>> >> I was reproducing with
>> >>
>> >>  ceph-fuse mnt
>> >>  mkdir -p mnt/a
>> >>  cd mnt
>> >>  .../pjd-fstest-20080816/fstest -u 65534 -g 65534 open a/foo O_RDONLY,O_CREAT 0644
>> >>
>> >> EACCES is expected, ENOENT was returned instead.
>> >
>> > I don't quite undersdand what is happending here.
>> >
>> > Why does ceph-fuse return ENOENT here?  Returning ENOENT for O_CREAT
>> > only makes sense if the parent directory doesn't exist, but that's not
>> > the case here.
>> 
>> More specifically, the initial lookup will return ENOENT, that's fine.
>> 
>> But in that case fuse_atomic_open() will do a FUSE_CREATE for which
>> ENOENT is not the right answer.
>
> I didn't dig deeply into what fuse_lookup() itself was doing, but the 
> short version is:
>
> - namei.c's atomic_open() gets -EACCES from the may_o_create() check, sets 
> create_error, and clears O_CREAT from the flags
>
> - fuse_atomic_open() calls fuse_lookup(), which returns NULL and leaves 
> entry->d_inode == NULL.  ceph-fuse's lookup method returned -ENOENT.  
> fuse_lookup() is just hashing the negative dentry instead of returning 
> ENOENT.
>
> - fuse_atomic_open() sees O_CREAT is not set and calls finish_no_open().
>
> - the existing ENOENT -> EACCES check in atomic_open() (below) fails 
> because fuse_atomic_open() returned 1:
>
> 	if (error < 0) {
> 		if (create_error && error == -ENOENT)
> 			error = create_error;
> 		goto out;
> 	}
>
> This patch just applies the same ENOENT->EACCES logic to the equivalent 
> negative/null dentry case.

Okay, I get it now.

Yes, your patch looks correct.  I'll queue it up with the other
atomic-open fixes.

Thanks,
Miklos

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/