Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3b] eventfd: convert to f_op->read_iter()
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <97a28bdb-284a-c215-c04d-288bcef66376@kernel.dk>
 <20200501190018.GN23230@ZenIV.linux.org.uk>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <db9405e8-5960-787f-5a4c-8266b2e456f2@kernel.dk>
Date:   Fri, 1 May 2020 13:08:18 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200501190018.GN23230@ZenIV.linux.org.uk>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 5/1/20 1:00 PM, Al Viro wrote:
> On Fri, May 01, 2020 at 11:54:01AM -0600, Jens Axboe wrote:
> 
>> @@ -427,8 +424,17 @@ static int do_eventfd(unsigned int count, int flags)
>>  
>>  	fd = anon_inode_getfd("[eventfd]", &eventfd_fops, ctx,
>>  			      O_RDWR | (flags & EFD_SHARED_FCNTL_FLAGS));
>> -	if (fd < 0)
>> +	if (fd < 0) {
>>  		eventfd_free_ctx(ctx);
>> +	} else {
>> +		struct file *file;
>> +
>> +		file = fget(fd);
>> +		if (file) {
>> +			file->f_mode |= FMODE_NOWAIT;
>> +			fput(file);
>> +		}
> 
> No.  The one and only thing you can do to return value of anon_inode_getfd() is to
> return the fscker to userland.  You *CAN* *NOT* assume that descriptor table is
> still pointing to whatever you've just created.
> 
> As soon as it's in descriptor table, it's out of your hands.  And frankly, if you
> are playing with descriptors, you should be very well aware of that.
> 
> Descriptor tables are fundamentally shared objects; they *can* be accessed and
> modified by other threads, right behind your back.
> 
> *IF* you are going to play with ->f_mode, you must use get_unused_fd_flags(),
> anon_inode_getfile(), modify ->f_mode of the result and use fd_install() to
> put it into descriptor table.  With put_unused_fd() as cleanup in case
> of allocation failure.

OK, that makes sense, so we've got f_mode set before the fd_install() and fd
visibility. I wrote that up, will test, and send out a v4... Thanks Al, this
is very helpful.

-- 
Jens Axboe