[PATCH] fs/vfs: Release allocated dentry on failure in do_last()

[PATCH] fs/vfs: Release allocated dentry on failure in do_last()

[穆阿浩(姜弋)]

This issue is found when creating /dev/sdtest with flags (O_CREAT |
O_DIRECT). The file still can be retrieved even after system reports
failure (-EINVAL) for it. Reporting error on creating the file is
correct behaviour because either devtmpfs or tmpfs doesn't support
O_DIRECT for regular file. However, it's incorrect that the file is
still existing. The cause is the newly allocated dentry and inode
aren't released on failure in do_last().

   # rm /dev/sdtest
   # dd if=/dev/urandom of=/dev/sdtest bs=4k count=1 oflag=direct
     <-EINVAL is returned>
   # ls /dev/sdtest
     <File is still existing>

This fixes the issue by releasing the dentry, thus the inode on failure
in do_last(). With this applied, the file (/dev/sdtest) isn't seen
in this scenario.

Reported-by: YouYou <youyou.wy.huang@foxconn.com>
Signed-off-by: Gavin Shan <dubo.sgw@alibaba-inc.com>
Signed-off-by: Liguang Zhang <liguang.zlg@alibaba-inc.com>
Signed-off-by: Ahao Mu <ahao.mah@alibaba-inc.com>
---
 fs/namei.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index 9cc91fb..607c357 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3382,6 +3382,8 @@ static int do_last(struct nameidata *nd,
 	*opened |= FILE_OPENED;
 opened:
 	error = open_check_o_direct(file);
+	if (error && (*opened & FILE_OPENED))
+		dput(path.dentry);
 	if (!error)
 		error = ima_file_check(file, op->acc_mode, *opened);
 	if (!error && will_truncate)
-- 
1.8.3.1

Re: [PATCH] fs/vfs: Release allocated dentry on failure in do_last()

[Matthew Wilcox]

On Thu, Dec 07, 2017 at 10:46:22AM +0800, 穆阿浩(姜弋) wrote:
> This issue is found when creating /dev/sdtest with flags (O_CREAT |
> O_DIRECT). The file still can be retrieved even after system reports
> failure (-EINVAL) for it. Reporting error on creating the file is
> correct behaviour because either devtmpfs or tmpfs doesn't support
> O_DIRECT for regular file. However, it's incorrect that the file is
> still existing. The cause is the newly allocated dentry and inode
> aren't released on failure in do_last().

Previously reported:

https://www.spinics.net/lists/linux-fsdevel/msg89317.html
https://www.spinics.net/lists/linux-fsdevel/msg113680.html
http://lkml.iu.edu/hypermail/linux/kernel/1609.0/04556.html

> +++ b/fs/namei.c
> @@ -3382,6 +3382,8 @@ static int do_last(struct nameidata *nd,
>  	*opened |= FILE_OPENED;
>  opened:
>  	error = open_check_o_direct(file);
> +	if (error && (*opened & FILE_OPENED))
> +		dput(path.dentry);
>  	if (!error)
>  		error = ima_file_check(file, op->acc_mode, *opened);
>  	if (!error && will_truncate)

Umm ... I think it's too late.  This will work well enough for in-memory
filesystems, but if you have a real filesystem, there's no call back to
the filesystem to remove the directory entry, right?

Re: [PATCH] fs/vfs: Release allocated dentry on failure in do_last()

[Al Viro]

On Thu, Dec 07, 2017 at 10:46:22AM +0800, 穆阿浩(姜弋) wrote:
> This issue is found when creating /dev/sdtest with flags (O_CREAT |
> O_DIRECT). The file still can be retrieved even after system reports
> failure (-EINVAL) for it. Reporting error on creating the file is
> correct behaviour because either devtmpfs or tmpfs doesn't support
> O_DIRECT for regular file. However, it's incorrect that the file is
> still existing. The cause is the newly allocated dentry and inode
> aren't released on failure in do_last().

>    # rm /dev/sdtest
>    # dd if=/dev/urandom of=/dev/sdtest bs=4k count=1 oflag=direct
>      <-EINVAL is returned>
>    # ls /dev/sdtest
>      <File is still existing>
> 
> This fixes the issue by releasing the dentry, thus the inode on failure
> in do_last(). With this applied, the file (/dev/sdtest) isn't seen
> in this scenario.

> +	if (error && (*opened & FILE_OPENED))
> +		dput(path.dentry);

NAK.  For one thing, it's racy as hell even on tmpfs - plain open()
from another process would've succeeded in that window.  For another,
it's outright exploitable on filesystems where dentry tree does not
contain all the existing directory tree (anything disk-based, for
starters).

Re: [PATCH] fs/vfs: Release allocated dentry on failure in do_last()

[渡波]

On Thu, Dec 07, 2017 at 08:39:37PM +0800, Al Viro wrote:
> On Thu, Dec 07, 2017 at 10:46:22AM +0800, 锟铰帮拷锟斤拷(锟斤拷弋) wrote:
> > This issue is found when creating /dev/sdtest with flags (O_CREAT |
> > O_DIRECT). The file still can be retrieved even after system reports
> > failure (-EINVAL) for it. Reporting error on creating the file is
> > correct behaviour because either devtmpfs or tmpfs doesn't support
> > O_DIRECT for regular file. However, it's incorrect that the file is
> > still existing. The cause is the newly allocated dentry and inode
> > aren't released on failure in do_last().
> 
> >    # rm /dev/sdtest
> >    # dd if=/dev/urandom of=/dev/sdtest bs=4k count=1 oflag=direct
> >      <-EINVAL is returned>
> >    # ls /dev/sdtest
> >      <File is still existing>
> > 
> > This fixes the issue by releasing the dentry, thus the inode on failure
> > in do_last(). With this applied, the file (/dev/sdtest) isn't seen
> > in this scenario.
> 
> > +	if (error && (*opened & FILE_OPENED))
> > +		dput(path.dentry);
> 
> NAK.  For one thing, it's racy as hell even on tmpfs - plain open()
> from another process would've succeeded in that window.  For another,
> it's outright exploitable on filesystems where dentry tree does not
> contain all the existing directory tree (anything disk-based, for
> starters).

Al, thanks for your reply. Another approach would be passing the flag
O_DIRECT to struct inode_operations::create(). The specific filesystem
rejects to create the inode if O_DIRECT isn't supported there. I can
send patches for review if you think it's reasonable.

Cheers,
Gavin

Re: [PATCH] fs/vfs: Release allocated dentry on failure in do_last()

[渡波]

On Thu, Dec 07, 2017 at 01:30:03PM +0800, Matthew Wilcox wrote:
> On Thu, Dec 07, 2017 at 10:46:22AM +0800, 锟铰帮拷锟斤拷(锟斤拷弋) wrote:
> > This issue is found when creating /dev/sdtest with flags (O_CREAT |
> > O_DIRECT). The file still can be retrieved even after system reports
> > failure (-EINVAL) for it. Reporting error on creating the file is
> > correct behaviour because either devtmpfs or tmpfs doesn't support
> > O_DIRECT for regular file. However, it's incorrect that the file is
> > still existing. The cause is the newly allocated dentry and inode
> > aren't released on failure in do_last().
> 
> Previously reported:
> 
> https://www.spinics.net/lists/linux-fsdevel/msg89317.html
> https://www.spinics.net/lists/linux-fsdevel/msg113680.html
> http://lkml.iu.edu/hypermail/linux/kernel/1609.0/04556.html
> 

Thanks, Matthew. I start to realize it's a long-standing issue :)

> > +++ b/fs/namei.c
> > @@ -3382,6 +3382,8 @@ static int do_last(struct nameidata *nd,
> >  	*opened |= FILE_OPENED;
> >  opened:
> >  	error = open_check_o_direct(file);
> > +	if (error && (*opened & FILE_OPENED))
> > +		dput(path.dentry);
> >  	if (!error)
> >  		error = ima_file_check(file, op->acc_mode, *opened);
> >  	if (!error && will_truncate)
> 
> Umm ... I think it's too late.  This will work well enough for in-memory
> filesystems, but if you have a real filesystem, there's no call back to
> the filesystem to remove the directory entry, right?

Yeah, it's late to roll back on real filesystem like ext4. As I replied
to Al Viro in another thread, the doable fix would be pass O_DIRECT down
to struct inode_operations::create() and the specific filesystem rejects
creating inode if O_DIRECT isn't supported there.

Cheers,
Gavin