Hi,

We recently had a failure on a kernel upgrade because splice no longer
works on random/urandom. This is due to:

6e2c7421f02 ("fs: don't allow splice read/write without explicit ops")

which already has more than two handful of Fixes registered to its
name...

Wire up read_iter handling and then hook up splice_read for both of
them as well.

-- 
Jens Axboe

Hi Jens,

On Thu, May 19, 2022 at 01:31:31PM -0600, Jens Axboe wrote:
> Hi,
> 
> We recently had a failure on a kernel upgrade because splice no longer
> works on random/urandom. This is due to:
> 
> 6e2c7421f02 ("fs: don't allow splice read/write without explicit ops")

Thanks for this. I'd noticed this a few months ago and assumed it has
just always been that way, and hadn't gotten to looking at what was up.

I'll take a look at these patches in detail when I'm home in a few
hours, but one thing maybe you can answer more easily than my digging
is:

There's a lot of attention in random.c devoted to not leaving any output
around on the stack or in stray buffers. The explicit use of
copy_to_user() makes it clear that the output isn't being copied
anywhere other than what's the user's responsibility to cleanup. I'm
wondering if the switch to copy_to_iter() introduces any buffering or
gotchas that you might be aware of.

Also you may need to rebase this on the random.git tree at
https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git

Regards,
Jason

On 5/19/22 2:05 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 01:31:31PM -0600, Jens Axboe wrote:
>> Hi,
>>
>> We recently had a failure on a kernel upgrade because splice no longer
>> works on random/urandom. This is due to:
>>
>> 6e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
> 
> Thanks for this. I'd noticed this a few months ago and assumed it has
> just always been that way, and hadn't gotten to looking at what was up.
> 
> I'll take a look at these patches in detail when I'm home in a few
> hours, but one thing maybe you can answer more easily than my digging
> is:

Sounds good, thanks!

> There's a lot of attention in random.c devoted to not leaving any output
> around on the stack or in stray buffers. The explicit use of
> copy_to_user() makes it clear that the output isn't being copied
> anywhere other than what's the user's responsibility to cleanup. I'm
> wondering if the switch to copy_to_iter() introduces any buffering or
> gotchas that you might be aware of.

No, it's just a wrapper around copying to the user memory pointed to by
the iov_iter. No extra buffering or anything like that. So I think it
should be fine in that respect, and it actually cleans up the code a bit
imho since the copy_to_iter() since the return value of "bytes copied"
is easier to work with than the "bytes not copied".

> Also you may need to rebase this on the random.git tree at
> https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git

OK, I will rebase it on that branch, not a problem.

-- 
Jens Axboe

Hi Jens,

On Thu, May 19, 2022 at 02:49:13PM -0600, Jens Axboe wrote:
> > There's a lot of attention in random.c devoted to not leaving any output
> > around on the stack or in stray buffers. The explicit use of
> > copy_to_user() makes it clear that the output isn't being copied
> > anywhere other than what's the user's responsibility to cleanup. I'm
> > wondering if the switch to copy_to_iter() introduces any buffering or
> > gotchas that you might be aware of.
> 
> No, it's just a wrapper around copying to the user memory pointed to by
> the iov_iter. No extra buffering or anything like that. So I think it
> should be fine in that respect, and it actually cleans up the code a bit
> imho since the copy_to_iter() since the return value of "bytes copied"
> is easier to work with than the "bytes not copied".

Alright, that's good to hear. So even for kernel->kernel writes, the
argument is that what ever buffers are used in the process are the same
ones that the user would be hitting anyway by calling write() on the
destination if this roundtripped through userspace, so nothing changes?

Jason

On 5/19/22 5:13 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 02:49:13PM -0600, Jens Axboe wrote:
>>> There's a lot of attention in random.c devoted to not leaving any output
>>> around on the stack or in stray buffers. The explicit use of
>>> copy_to_user() makes it clear that the output isn't being copied
>>> anywhere other than what's the user's responsibility to cleanup. I'm
>>> wondering if the switch to copy_to_iter() introduces any buffering or
>>> gotchas that you might be aware of.
>>
>> No, it's just a wrapper around copying to the user memory pointed to by
>> the iov_iter. No extra buffering or anything like that. So I think it
>> should be fine in that respect, and it actually cleans up the code a bit
>> imho since the copy_to_iter() since the return value of "bytes copied"
>> is easier to work with than the "bytes not copied".
> 
> Alright, that's good to hear. So even for kernel->kernel writes, the
> argument is that what ever buffers are used in the process are the same
> ones that the user would be hitting anyway by calling write() on the
> destination if this roundtripped through userspace, so nothing changes?

The source and destination for the copies are exactly the same with the
change as before, so no changes there. The non-user copy is a different
helper.

-- 
Jens Axboe

Hi Jens,

On Fri, May 20, 2022 at 1:19 AM Jens Axboe <axboe@kernel.dk> wrote:
> The source and destination for the copies are exactly the same with the
> change as before, so no changes there. The non-user copy is a different
> helper.

Oh, okay. Maybe a silly question but: should we wire up that helper
too? (If I'm understanding your meaning right.) Seems like it'd be a
good idea to just wire up all the things while we're at it.

Jason

On 5/19/22 5:23 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Fri, May 20, 2022 at 1:19 AM Jens Axboe <axboe@kernel.dk> wrote:
>> The source and destination for the copies are exactly the same with the
>> change as before, so no changes there. The non-user copy is a different
>> helper.
> 
> Oh, okay. Maybe a silly question but: should we wire up that helper
> too? (If I'm understanding your meaning right.) Seems like it'd be a
> good idea to just wire up all the things while we're at it.

I'll leave that to you :-)

I'll do the write_iter though, just for completeness.

-- 
Jens Axboe

Hi Jens,

On Fri, May 20, 2022 at 1:25 AM Jens Axboe <axboe@kernel.dk> wrote:
> I'll leave that to you :-)

Alright, I'll have a look. Which one do I want here? This series adds
splice_read/splice_write. The new thing would be sendpage? Or
copy_file_range? Or something else?

Jason

On 5/19/22 5:27 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Fri, May 20, 2022 at 1:25 AM Jens Axboe <axboe@kernel.dk> wrote:
>> I'll leave that to you :-)
> 
> Alright, I'll have a look. Which one do I want here? This series adds
> splice_read/splice_write. The new thing would be sendpage? Or
> copy_file_range? Or something else?

For copying kernel memory? It's really the same thing, you just
initialize the iter as an ITER_KVEC instead. The nice thing about the
iov_iter iterator that it then hides that for you, call the same copy
in/out helpers for it.

-- 
Jens Axboe

Hi Jens,

On Fri, May 20, 2022 at 1:57 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 5/19/22 5:27 PM, Jason A. Donenfeld wrote:
> > Hi Jens,
> >
> > On Fri, May 20, 2022 at 1:25 AM Jens Axboe <axboe@kernel.dk> wrote:
> >> I'll leave that to you :-)
> >
> > Alright, I'll have a look. Which one do I want here? This series adds
> > splice_read/splice_write. The new thing would be sendpage? Or
> > copy_file_range? Or something else?
>
> For copying kernel memory? It's really the same thing, you just
> initialize the iter as an ITER_KVEC instead. The nice thing about the
> iov_iter iterator that it then hides that for you, call the same copy
> in/out helpers for it.

Err, maybe we're talking about different things? I was thinking about
the plumbing to make splice/sendfile work on non-pipes.

get_random_bytes() itself doesn't need to become iovec'd, as that has
no IO callers.

Jason

On 5/19/22 6:00 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Fri, May 20, 2022 at 1:57 AM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 5/19/22 5:27 PM, Jason A. Donenfeld wrote:
>>> Hi Jens,
>>>
>>> On Fri, May 20, 2022 at 1:25 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>> I'll leave that to you :-)
>>>
>>> Alright, I'll have a look. Which one do I want here? This series adds
>>> splice_read/splice_write. The new thing would be sendpage? Or
>>> copy_file_range? Or something else?
>>
>> For copying kernel memory? It's really the same thing, you just
>> initialize the iter as an ITER_KVEC instead. The nice thing about the
>> iov_iter iterator that it then hides that for you, call the same copy
>> in/out helpers for it.
> 
> Err, maybe we're talking about different things? I was thinking about
> the plumbing to make splice/sendfile work on non-pipes.

Ah I see. sendfile() just uses splice() internally, so that'll work
(again) with my changes. splice(), by definition, either moves to and
from a pipe. Hence one of the fds must be a pipe. Does that answer the
question?

> get_random_bytes() itself doesn't need to become iovec'd, as that has
> no IO callers.

I was a little puzzled, but didn't look deeper and see if it would make
sense to do so.

-- 
Jens Axboe

Hi Jens,

On Thu, May 19, 2022 at 06:02:55PM -0600, Jens Axboe wrote:
> On 5/19/22 6:00 PM, Jason A. Donenfeld wrote:
> > Hi Jens,
> > 
> > On Fri, May 20, 2022 at 1:57 AM Jens Axboe <axboe@kernel.dk> wrote:
> >>
> >> On 5/19/22 5:27 PM, Jason A. Donenfeld wrote:
> >>> Hi Jens,
> >>>
> >>> On Fri, May 20, 2022 at 1:25 AM Jens Axboe <axboe@kernel.dk> wrote:
> >>>> I'll leave that to you :-)
> >>>
> >>> Alright, I'll have a look. Which one do I want here? This series adds
> >>> splice_read/splice_write. The new thing would be sendpage? Or
> >>> copy_file_range? Or something else?
> >>
> >> For copying kernel memory? It's really the same thing, you just
> >> initialize the iter as an ITER_KVEC instead. The nice thing about the
> >> iov_iter iterator that it then hides that for you, call the same copy
> >> in/out helpers for it.
> > 
> > Err, maybe we're talking about different things? I was thinking about
> > the plumbing to make splice/sendfile work on non-pipes.
> 
> Ah I see. sendfile() just uses splice() internally, so that'll work
> (again) with my changes. splice(), by definition, either moves to and
> from a pipe. Hence one of the fds must be a pipe. Does that answer the
> question?

sendfile() returns -EINVAL even with your patches. Only splicing to pipes
seems to work.

Jason

On 5/19/22 6:48 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 06:02:55PM -0600, Jens Axboe wrote:
>> On 5/19/22 6:00 PM, Jason A. Donenfeld wrote:
>>> Hi Jens,
>>>
>>> On Fri, May 20, 2022 at 1:57 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> On 5/19/22 5:27 PM, Jason A. Donenfeld wrote:
>>>>> Hi Jens,
>>>>>
>>>>> On Fri, May 20, 2022 at 1:25 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> I'll leave that to you :-)
>>>>>
>>>>> Alright, I'll have a look. Which one do I want here? This series adds
>>>>> splice_read/splice_write. The new thing would be sendpage? Or
>>>>> copy_file_range? Or something else?
>>>>
>>>> For copying kernel memory? It's really the same thing, you just
>>>> initialize the iter as an ITER_KVEC instead. The nice thing about the
>>>> iov_iter iterator that it then hides that for you, call the same copy
>>>> in/out helpers for it.
>>>
>>> Err, maybe we're talking about different things? I was thinking about
>>> the plumbing to make splice/sendfile work on non-pipes.
>>
>> Ah I see. sendfile() just uses splice() internally, so that'll work
>> (again) with my changes. splice(), by definition, either moves to and
>> from a pipe. Hence one of the fds must be a pipe. Does that answer the
>> question?
> 
> sendfile() returns -EINVAL even with your patches. Only splicing to pipes
> seems to work.

Huh, that really should work. Are you trying to sendfile() to random? If
so, you need that last write_iter patch too, and add the splice_write as
I mentioned.

-- 
Jens Axboe

Hi Jens,

On Thu, May 19, 2022 at 06:56:12PM -0600, Jens Axboe wrote:
> On 5/19/22 6:48 PM, Jason A. Donenfeld wrote:
> > sendfile() returns -EINVAL even with your patches. Only splicing to pipes
> > seems to work.
> 
> Huh, that really should work. Are you trying to sendfile() to random? If
> so, you need that last write_iter patch too, and add the splice_write as
> I mentioned.
 
No, I've only tried the read side so far. I made a little program:

#include <sys/sendfile.h>
#include <stdio.h>

int main(int argc, char *argv[])
{
        ssize_t s = sendfile(1, 0, NULL, 0xffff);
        fprintf(stderr, "ret: %zd\n", s);
        return 0;
}

Then I ran `./a.out < /dev/urandom > /dev/null`. Fails. OTOH, if I
replace /dev/urandom with an ordinary file, it succeeds.

Jason

On 5/19/22 7:00 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 06:56:12PM -0600, Jens Axboe wrote:
>> On 5/19/22 6:48 PM, Jason A. Donenfeld wrote:
>>> sendfile() returns -EINVAL even with your patches. Only splicing to pipes
>>> seems to work.
>>
>> Huh, that really should work. Are you trying to sendfile() to random? If
>> so, you need that last write_iter patch too, and add the splice_write as
>> I mentioned.
>  
> No, I've only tried the read side so far. I made a little program:
> 
> #include <sys/sendfile.h>
> #include <stdio.h>
> 
> int main(int argc, char *argv[])
> {
>         ssize_t s = sendfile(1, 0, NULL, 0xffff);
>         fprintf(stderr, "ret: %zd\n", s);
>         return 0;
> }
> 
> Then I ran `./a.out < /dev/urandom > /dev/null`. Fails. OTOH, if I
> replace /dev/urandom with an ordinary file, it succeeds.

Here's why, it's limited to regular files or block devices:

if (unlikely(!S_ISREG(i_mode) && !S_ISBLK(i_mode)))
	return -EINVAL;

in splice_direct_to_actor().


-- 
Jens Axboe

Hi Jens,

On Thu, May 19, 2022 at 07:10:56PM -0600, Jens Axboe wrote:
> On 5/19/22 7:00 PM, Jason A. Donenfeld wrote:
> > Hi Jens,
> > 
> > On Thu, May 19, 2022 at 06:56:12PM -0600, Jens Axboe wrote:
> >> On 5/19/22 6:48 PM, Jason A. Donenfeld wrote:
> >>> sendfile() returns -EINVAL even with your patches. Only splicing to pipes
> >>> seems to work.
> >>
> >> Huh, that really should work. Are you trying to sendfile() to random? If
> >> so, you need that last write_iter patch too, and add the splice_write as
> >> I mentioned.
> >  
> > No, I've only tried the read side so far. I made a little program:
> > 
> > #include <sys/sendfile.h>
> > #include <stdio.h>
> > 
> > int main(int argc, char *argv[])
> > {
> >         ssize_t s = sendfile(1, 0, NULL, 0xffff);
> >         fprintf(stderr, "ret: %zd\n", s);
> >         return 0;
> > }
> > 
> > Then I ran `./a.out < /dev/urandom > /dev/null`. Fails. OTOH, if I
> > replace /dev/urandom with an ordinary file, it succeeds.
> 
> Here's why, it's limited to regular files or block devices:
> 
> if (unlikely(!S_ISREG(i_mode) && !S_ISBLK(i_mode)))
> 	return -EINVAL;
> 
> in splice_direct_to_actor().

Indeed. Looks like that was your code from long long ago!

I posted
https://lore.kernel.org/lkml/20220520095747.123748-1-Jason@zx2c4.com/ to
fix it if you'd like to review.

Jason

On 5/20/22 6:43 AM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 07:10:56PM -0600, Jens Axboe wrote:
>> On 5/19/22 7:00 PM, Jason A. Donenfeld wrote:
>>> Hi Jens,
>>>
>>> On Thu, May 19, 2022 at 06:56:12PM -0600, Jens Axboe wrote:
>>>> On 5/19/22 6:48 PM, Jason A. Donenfeld wrote:
>>>>> sendfile() returns -EINVAL even with your patches. Only splicing to pipes
>>>>> seems to work.
>>>>
>>>> Huh, that really should work. Are you trying to sendfile() to random? If
>>>> so, you need that last write_iter patch too, and add the splice_write as
>>>> I mentioned.
>>>  
>>> No, I've only tried the read side so far. I made a little program:
>>>
>>> #include <sys/sendfile.h>
>>> #include <stdio.h>
>>>
>>> int main(int argc, char *argv[])
>>> {
>>>         ssize_t s = sendfile(1, 0, NULL, 0xffff);
>>>         fprintf(stderr, "ret: %zd\n", s);
>>>         return 0;
>>> }
>>>
>>> Then I ran `./a.out < /dev/urandom > /dev/null`. Fails. OTOH, if I
>>> replace /dev/urandom with an ordinary file, it succeeds.
>>
>> Here's why, it's limited to regular files or block devices:
>>
>> if (unlikely(!S_ISREG(i_mode) && !S_ISBLK(i_mode)))
>> 	return -EINVAL;
>>
>> in splice_direct_to_actor().
> 
> Indeed. Looks like that was your code from long long ago!

Yep I would not be surprised if that is the case!

> I posted
> https://lore.kernel.org/lkml/20220520095747.123748-1-Jason@zx2c4.com/ to
> fix it if you'd like to review.

Not in my inbox, but you also used an email that hasn't been valid in 16
years :-)

But looks fine to me, we can open this up to character devices, don't
see an issue with that.

-- 
Jens Axboe

Hi Jens,

On Fri, May 20, 2022 at 06:49:45AM -0600, Jens Axboe wrote:
> > I posted
> > https://lore.kernel.org/lkml/20220520095747.123748-1-Jason@zx2c4.com/ to
> > fix it if you'd like to review.
> 
> Not in my inbox, but you also used an email that hasn't been valid in 16
> years :-)
> 
> But looks fine to me, we can open this up to character devices, don't
> see an issue with that.

Whoops! I'll bounce it to you so you can provide a Reviewed-by for Al
over there.

Jason

On 5/19/22 7:00 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 06:56:12PM -0600, Jens Axboe wrote:
>> On 5/19/22 6:48 PM, Jason A. Donenfeld wrote:
>>> sendfile() returns -EINVAL even with your patches. Only splicing to pipes
>>> seems to work.
>>
>> Huh, that really should work. Are you trying to sendfile() to random? If
>> so, you need that last write_iter patch too, and add the splice_write as
>> I mentioned.
>  
> No, I've only tried the read side so far. I made a little program:
> 
> #include <sys/sendfile.h>
> #include <stdio.h>
> 
> int main(int argc, char *argv[])
> {
>         ssize_t s = sendfile(1, 0, NULL, 0xffff);
>         fprintf(stderr, "ret: %zd\n", s);
>         return 0;
> }
> 
> Then I ran `./a.out < /dev/urandom > /dev/null`. Fails. OTOH, if I
> replace /dev/urandom with an ordinary file, it succeeds.

Yeah I reproduced the same, doing it the other way works fine. Curious
enough to see what it is, looking into it.

-- 
Jens Axboe

On 5/19/22 2:49 PM, Jens Axboe wrote:
> On 5/19/22 2:05 PM, Jason A. Donenfeld wrote:
>> Hi Jens,
>>
>> On Thu, May 19, 2022 at 01:31:31PM -0600, Jens Axboe wrote:
>>> Hi,
>>>
>>> We recently had a failure on a kernel upgrade because splice no longer
>>> works on random/urandom. This is due to:
>>>
>>> 6e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
>>
>> Thanks for this. I'd noticed this a few months ago and assumed it has
>> just always been that way, and hadn't gotten to looking at what was up.
>>
>> I'll take a look at these patches in detail when I'm home in a few
>> hours, but one thing maybe you can answer more easily than my digging
>> is:
> 
> Sounds good, thanks!
> 
>> There's a lot of attention in random.c devoted to not leaving any output
>> around on the stack or in stray buffers. The explicit use of
>> copy_to_user() makes it clear that the output isn't being copied
>> anywhere other than what's the user's responsibility to cleanup. I'm
>> wondering if the switch to copy_to_iter() introduces any buffering or
>> gotchas that you might be aware of.
> 
> No, it's just a wrapper around copying to the user memory pointed to by
> the iov_iter. No extra buffering or anything like that. So I think it
> should be fine in that respect, and it actually cleans up the code a bit
> imho since the copy_to_iter() since the return value of "bytes copied"
> is easier to work with than the "bytes not copied".
> 
>> Also you may need to rebase this on the random.git tree at
>> https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git
> 
> OK, I will rebase it on that branch, not a problem.

Rebased patches attached, you can also find them here:

https://git.kernel.dk/cgit/linux-block/log/?h=random-splice

Did some basic sanity checking (and with splice too), and seems fine
rebased as well.

-- 
Jens Axboe

Hi Jens,

On Thu, May 19, 2022 at 03:02:28PM -0600, Jens Axboe wrote:
> Rebased patches attached, you can also find them here:
> 
> https://git.kernel.dk/cgit/linux-block/log/?h=random-splice
> 
> Did some basic sanity checking (and with splice too), and seems fine
> rebased as well.

Thanks. I left one comment about patch 1 in that subthread. The general
idea of this patchset seems fine, but: what about write_iter? Can't we
convert both of them? 1/3 - read_iter, 2/3 - write_iter, 3/3 - add the
generic splice helpers. I ask because it seems weird to keep around the
old thing (which sounds like is being gradually removed?) alongside the
new thing.

Jason

On 5/19/22 5:15 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 03:02:28PM -0600, Jens Axboe wrote:
>> Rebased patches attached, you can also find them here:
>>
>> https://git.kernel.dk/cgit/linux-block/log/?h=random-splice
>>
>> Did some basic sanity checking (and with splice too), and seems fine
>> rebased as well.
> 
> Thanks. I left one comment about patch 1 in that subthread. The general
> idea of this patchset seems fine, but: what about write_iter? Can't we
> convert both of them? 1/3 - read_iter, 2/3 - write_iter, 3/3 - add the
> generic splice helpers. I ask because it seems weird to keep around the
> old thing (which sounds like is being gradually removed?) alongside the
> new thing.

I can certainly do the write side too. To fix this regression, I just
valued doing read_iter first and I'd hate to hold that up to do the
write side too. I'll do the write side later today, but let's keep them
separate.

In general, everyone using ->read and ->write should be converted so we
can kill these handlers. Would be great if someone would take that on...

-- 
Jens Axboe

Hi Jens,

On Fri, May 20, 2022 at 1:22 AM Jens Axboe <axboe@kernel.dk> wrote:
> I can certainly do the write side too. To fix this regression, I just
> valued doing read_iter first and I'd hate to hold that up to do the
> write side too. I'll do the write side later today, but let's keep them
> separate.

Excellent, thanks. I plan to queue these up all in a row.

Jason

On 5/19/22 5:25 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Fri, May 20, 2022 at 1:22 AM Jens Axboe <axboe@kernel.dk> wrote:
>> I can certainly do the write side too. To fix this regression, I just
>> valued doing read_iter first and I'd hate to hold that up to do the
>> write side too. I'll do the write side later today, but let's keep them
>> separate.
> 
> Excellent, thanks. I plan to queue these up all in a row.

Built and tested v2, just sent it out. Note that it deviates from your
proposal a bit since with that we lost the

if (!len)
	break;

check, which is kind of important if you ever want to be done :-)

I'll do the write_iter side, but as mentioned, I'd prefer to keep it
separate from this patchset as this one fixes a real regression that we
need to get backported too.

-- 
Jens Axboe

Hi Jens,

On Thu, May 19, 2022 at 05:33:01PM -0600, Jens Axboe wrote:
> On 5/19/22 5:25 PM, Jason A. Donenfeld wrote:
> > Hi Jens,
> > 
> > On Fri, May 20, 2022 at 1:22 AM Jens Axboe <axboe@kernel.dk> wrote:
> >> I can certainly do the write side too. To fix this regression, I just
> >> valued doing read_iter first and I'd hate to hold that up to do the
> >> write side too. I'll do the write side later today, but let's keep them
> >> separate.
> > 
> > Excellent, thanks. I plan to queue these up all in a row.
> 
> Built and tested v2, just sent it out. Note that it deviates from your
> proposal a bit since with that we lost the
> 
> if (!len)
> 	break;
> 
> check, which is kind of important if you ever want to be done :-)

Heh, noticed that too, thanks.

> I'll do the write_iter side, but as mentioned, I'd prefer to keep it
> separate from this patchset as this one fixes a real regression that we
> need to get backported too.
 
No problem. Because of all the flux in random.c lately, I've been
preparing a massive backports branch, 2 branches actually, so I'll make
sure this is in there. Backport concern aside, though, I'll look for
your write_iter patch today. Thanks a bunch for doing this.

Jason

On 5/19/22 5:39 PM, Jason A. Donenfeld wrote:
> Hi Jens,
> 
> On Thu, May 19, 2022 at 05:33:01PM -0600, Jens Axboe wrote:
>> On 5/19/22 5:25 PM, Jason A. Donenfeld wrote:
>>> Hi Jens,
>>>
>>> On Fri, May 20, 2022 at 1:22 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>> I can certainly do the write side too. To fix this regression, I just
>>>> valued doing read_iter first and I'd hate to hold that up to do the
>>>> write side too. I'll do the write side later today, but let's keep them
>>>> separate.
>>>
>>> Excellent, thanks. I plan to queue these up all in a row.
>>
>> Built and tested v2, just sent it out. Note that it deviates from your
>> proposal a bit since with that we lost the
>>
>> if (!len)
>> 	break;
>>
>> check, which is kind of important if you ever want to be done :-)
> 
> Heh, noticed that too, thanks.

:-)

>> I'll do the write_iter side, but as mentioned, I'd prefer to keep it
>> separate from this patchset as this one fixes a real regression that we
>> need to get backported too.
>  
> No problem. Because of all the flux in random.c lately, I've been
> preparing a massive backports branch, 2 branches actually, so I'll make
> sure this is in there. Backport concern aside, though, I'll look for
> your write_iter patch today. Thanks a bunch for doing this.

Sounds great, thanks - write patch has been sent out too.

-- 
Jens Axboe

On Thu, May 19, 2022 at 01:31:31PM -0600, Jens Axboe wrote:
> Hi,
> 
> We recently had a failure on a kernel upgrade because splice no longer
> works on random/urandom. This is due to:
> 
> 6e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
> 
> which already has more than two handful of Fixes registered to its
> name...

Yes.  It was a hard break to get rid of set_fs and it's abuse.

On 5/19/22 1:48 PM, Christoph Hellwig wrote:
> On Thu, May 19, 2022 at 01:31:31PM -0600, Jens Axboe wrote:
>> Hi,
>>
>> We recently had a failure on a kernel upgrade because splice no longer
>> works on random/urandom. This is due to:
>>
>> 6e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
>>
>> which already has more than two handful of Fixes registered to its
>> name...
> 
> Yes.  It was a hard break to get rid of set_fs and it's abuse.

I'm a bit torn on this one, because I _really_ want us to get rid of
read/write and make everything use read_iter/write_iter. Firstly because
it's really stupid to have two interfaces, and secondly because even
basic things like "can we block here" doesn't work in the older
interface without fiddling with file flags which is a non-starter for
certain things.

However, it's also problematic that we end up breaking real applications
because of this change. Arguably we should've converted existing
read/write first and avoid this grey zone we are in now (best solution),
or provided splice read/write helpers that work with the non-iter based
read/write handlers.

-- 
Jens Axboe

On Thu, May 19, 2022 at 01:55:26PM -0600, Jens Axboe wrote:
> I'm a bit torn on this one, because I _really_ want us to get rid of
> read/write and make everything use read_iter/write_iter. Firstly because
> it's really stupid to have two interfaces, and secondly because even
> basic things like "can we block here" doesn't work in the older
> interface without fiddling with file flags which is a non-starter for
> certain things.

Converting everything was my initial plan, but Linus said no and just
fix whatever breaks.  And compared to my initial fears the fallout
actually isn't that bad.

On 5/20/22 12:02 AM, Christoph Hellwig wrote:
> On Thu, May 19, 2022 at 01:55:26PM -0600, Jens Axboe wrote:
>> I'm a bit torn on this one, because I _really_ want us to get rid of
>> read/write and make everything use read_iter/write_iter. Firstly because
>> it's really stupid to have two interfaces, and secondly because even
>> basic things like "can we block here" doesn't work in the older
>> interface without fiddling with file flags which is a non-starter for
>> certain things.
> 
> Converting everything was my initial plan, but Linus said no and just
> fix whatever breaks.  And compared to my initial fears the fallout
> actually isn't that bad.

That's a real shame, and very unlike Linus to advocate for just breaking
applications willy nilly. It would be a considerable amount of work
though, but it would really be great to not have ->read/->write anymore
and solely use the iter variants.

Not sure if I agree with "that bad" though looking at the number of
fixups, and we'll keep finding these going forward as well...

-- 
Jens Axboe