Received-SPF: pass (google.com: domain of linux-kernel+bounces-131472-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <8de27ff2-d53f-4900-b755-6a05bff98f78@kernel.dk>
Date: Thu, 4 Apr 2024 06:52:32 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/3] signalfd: convert to ->read_iter()
Content-Language: en-US
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org, brauner@kernel.org,
 linux-kernel@vger.kernel.org
References: <20240403140446.1623931-1-axboe@kernel.dk>
 <20240403140446.1623931-4-axboe@kernel.dk> <20240403225747.GO538574@ZenIV>
From: Jens Axboe <axboe@kernel.dk>
In-Reply-To: <20240403225747.GO538574@ZenIV>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 4/3/24 4:57 PM, Al Viro wrote:
> On Wed, Apr 03, 2024 at 08:02:54AM -0600, Jens Axboe wrote:
>> Rather than use the older style ->read() hook, use ->read_iter() so that
>> signalfd can support both O_NONBLOCK and IOCB_NOWAIT for non-blocking
>> read attempts.
>>
>> Split the fd setup into two parts, so that signalfd can mark the file
>> mode with FMODE_NOWAIT before installing it into the process table.
> 
> Same issue with copy_to_iter() calling conventions; what's more, userland
> really does not expect partial copies here, so it might be worth adding
> 
> static inline
> bool copy_to_iter_full(void *addr, size_t bytes, struct iov_iter *i)
> {
>         size_t copied = copy_to_iter(addr, bytes, i);
> 	if (likely(copied == bytes))
> 		return true;
> 	iov_iter_revert(i, copied);
> 	return false;
> }
> 
> to include/linux/uio.h for the sake of those suckers.  Then
> they could go for
>         return copy_to_iter_full(&new, sizeof(new), to) ? sizeof(new) : -EFAULT;
> and similar in other two.

That's a good idea, makes the transformations easier too and avoids
dealine with the partial case.

> NOTE: the userland ABI is somewhat sucky here - if the buffer goes
> unmapped (or r/o) at the offset that is *not* a multiple of
> sizeof(struct signalfd_siginfo), you get an event quietly lost.
> Not sure what can be done with that - it is a user-visible ABI.

The ABI could be nicer, but isn't this a case of "well don't do that
then"?

-- 
Jens Axboe