linux-kernel - Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJfpegsLeuw55Diw0yDmiFR+7qkSkOrW2fUbjmzFC9OxVsrAkQ@mail.gmail.com>
Date:	Mon, 16 Sep 2013 22:09:46 +0200
From:	Miklos Szeredi <miklos@...redi.hu>
To:	Al Viro <viro@...iv.linux.org.uk>
Cc:	Linux-Fsdevel <linux-fsdevel@...r.kernel.org>,
	Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"mszeredi@...e.cz" <mszeredi@...e.cz>,
	Eric Van Hensbergen <ericvh@...il.com>,
	"M. Mohan Kumar" <mohan@...ibm.com>, stable@...r.kernel.org
Subject: Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()

On Mon, Sep 16, 2013 at 9:50 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
> On Mon, Sep 16, 2013 at 09:03:25PM +0200, Miklos Szeredi wrote:
>> On Mon, Sep 16, 2013 at 8:19 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
>> > On Mon, Sep 16, 2013 at 02:51:56PM +0200, Miklos Szeredi wrote:
>> >> From: Miklos Szeredi <mszeredi@...e.cz>
>> >>
>> >> commit b6f4bee02f "fs/9p: Fix atomic_open" fixed the O_EXCL behavior, but
>> >> results in a dentry leak if v9fs_vfs_lookup() returns non-NULL.
>> >
>> > Frankly, I would prefer to deal with that in fs/namei.c:atomic_open()
>> > instead.  I.e. let it call finish_no_open() as it used to do and
>> > turn
>> >                 if (create_error && dentry->d_inode == NULL) {
>> >                         error = create_error;
>> >                         goto out;
>> >                 }
>> > in fs/namei.c:atomic_open() into
>> >                 if (!dentry->d_inode) {
>> >                         if (create_error) {
>> >                                 error = create_error;
>> >                                 goto out;
>> >                         }
>> >                 } else if ((open_flag & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL)) {
>> >                         error = -EEXIST;
>> >                         goto out;
>> >                 }
>> >
>> > rather than try to deal with that crap in each instance of ->atomic_open()...
>> > Objections?
>>
>> ->atomic_open() could be any one of
>>
>>  lookup
>>  lookup+create
>>  lookup+create+open
>>
>> If it's the second one then the above is wrong.  Sure, we could check
>> FILE_CREATED as well, and if file wasn't created yet dentry is
>> positive then we return EEXIST.  But for that to be correct we need
>> the last patch in the series, preventing FILE_CREATED from being set
>> unconditionally.
>
> You mean, lookup + create + return finish_no_open()?  Does anything actually
> do that?

Fuse does.

>  I agree that we want your "deal with setting FILE_CREATED in
> filesystems", BTW, and I'm fine with putting it in front of the rest of
> the queue.
>
> I would definitely prefer EEXIST logics dealt with in fs/namei.c - if nothing
> else, it had been done wrong in too many instances...

Okay.

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/