linux-kernel - Re: [PATCHSET RFC 0/437] Kill off old fops ->read() and ->write()

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]

Date: Sat, 13 Apr 2024 09:37:23 -0600
From: Jens Axboe <axboe@...nel.dk>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCHSET RFC 0/437] Kill off old fops ->read() and ->write()

On 4/12/24 10:15 PM, Al Viro wrote:
> On Fri, Apr 12, 2024 at 07:58:13AM -0600, Jens Axboe wrote:
> 
>> I'm aware of some drivers that do different things from write vs writev,
>> or read vs readv for instance. But those I did cater to, by having a
>> flag they can now check.
>>
>> Can you be a bit more specific on an example of a driver that does the
>> above?
> 
> Consider e.g. your #39.  Current mainline: 1 call of ->set() for each
> segment passed to writev() on any of those.  With your patch: call
> segments concatenated and if the concatenation looks like a number, a
> single call of ->set().
> 
> If nothing else, it's a user-visible ABI change.  And in cases when
> ->set() has non-trivial side effects, it just might break a real-world
> code that is currently correct.
> 
> I picked that one because I didn't want to dig through the drivers -
> I'm pretty sure that there's more to be found there.
> 
> It's not just "write() and writev() parse the data in different way" -
> we do have a couple of those, but that's a minor problem.
> 
> "write(fd, buf, len1); write(fd, buf + len1, len1 + len2); is not the
> same thing as write(fd, buf, len1 + len2)" is not rare for character
> devices and for regular files on procfs/debugfs/etc.
> 
> For any of those you need to use you vfs_write_iter() helper or you'll
> be breaking userland ABI.  The cost of audit of several thousands of
> ->write() (and ->read() - similar problem applies there) instances,
> checking that property is the main reason this conversion hadn't been
> already done.

Thanks Al, I see what you mean now, and yes  that certainly makes
sense. I'll go over the conversions and see what might be affected, and
for those we'll probably just play it safe initially and use the FOPS
iterators helpers to avoid changing behavior.

-- 
Jens Axboe