linux-kernel - Re: [PATCH v4 0/3] random: convert to using iters, for Al Viro

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: <69bd18e6-d216-dfb3-201b-f6a285deb0e7@kernel.dk>
Date:   Fri, 20 May 2022 10:24:36 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     "Jason A. Donenfeld" <Jason@...c4.com>,
        Theodore Ts'o <tytso@....edu>, Christoph Hellwig <hch@....de>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 0/3] random: convert to using iters, for Al Viro

On 5/20/22 10:15 AM, Al Viro wrote:
> On Fri, May 20, 2022 at 09:53:30AM -0600, Jens Axboe wrote:
>> On 5/20/22 9:47 AM, Al Viro wrote:
>>> On Fri, May 20, 2022 at 09:34:46AM -0600, Jens Axboe wrote:
>>>
>>>> I'm very sure, otherwise we're just accepting that we're breaking real
>>>> world applications.
>>>
>>> "Breaking" as in "it used to work with earlier kernels, doesn't work with
>>> recent ones"?  Details, please...
>>
>> Yes, as in exactly that. This is what drove this addition of
>> ->read_iter() for urandom. See commit:
>>
>> ommit 36e2c7421f02a22f71c9283e55fdb672a9eb58e7
>> Author: Christoph Hellwig <hch@....de>
>> Date:   Thu Sep 3 16:22:34 2020 +0200
>>
>>     fs: don't allow splice read/write without explicit ops
>>
>> related to the set_fs() changes, and now go look for any commit that
>> has:
>>
>> Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
>>
>> in it and see that this isn't an isolated incident at all.
>>
>> tldr - splice from /dev/urandom used to work, and I recently got a
>> report internally on an application that broke on upgrade from 5.6 to
>> 5.12 exactly because it now just just -EINVAL's instead.
> 
> IIRC, Linus' position at the time had been along the lines of
> "splice is not so good ABI anyway, so let's do it and fix up
> the places that do get real-world complaints once such appear".
> So /dev/urandom is one such place...

That's what Christoph said too. Honestly that's a very odd way to
attempt to justify breakage like this, even if it is tempting to
facilitate the set_fs() removal. But then be honest about it and say
it like it is, rather than some hand wavy explanation that frankly
doesn't make any sense.

The referenced change doesn't change the splice ABI at all, hence the
justification seems very random to me. It kept what we already have,
except we randomly break some use cases.

-- 
Jens Axboe