PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Siddharth Gupta]

Hi all,

With the introduction of the filesystem change "fs: don't allow splice 
read/write without explicit ops"[1] the fallback mechanism of the 
firmware loader[2] no longer works when using sendfile[3] from the 
userspace.

Since the binary attributes don't support splice_{read,write} functions 
the calls to splice_{read,write} used the default kernel_{read,write} 
functions. With the above change this results in an -EINVAL return from 
do_splice_from[4].

This essentially means that sendfile will not work for any binary 
attribute in the sysfs.

Thanks,
Siddharth

[1]: 
https://github.com/torvalds/linux/commit/36e2c7421f02a22f71c9283e55fdb672a9eb58e7#diff-70c49af2ed5805fc1406ed6e6532d6a029ada1abd90cca6442711b9cecd4d523
[2]: 
https://github.com/torvalds/linux/blob/master/drivers/base/firmware_loader/main.c#L831
[3]: https://github.com/torvalds/linux/blob/master/fs/read_write.c#L1257
[4]: https://github.com/torvalds/linux/blob/master/fs/splice.c#L753

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Greg KH]

On Mon, Jan 04, 2021 at 02:43:45PM -0800, Siddharth Gupta wrote:
> Hi all,
> 
> With the introduction of the filesystem change "fs: don't allow splice
> read/write without explicit ops"[1] the fallback mechanism of the firmware
> loader[2] no longer works when using sendfile[3] from the userspace.

What userspace program are you using to load firmware?  Are you not
using the in-kernel firmware loader for some reason?

> Since the binary attributes don't support splice_{read,write} functions the
> calls to splice_{read,write} used the default kernel_{read,write} functions.
> With the above change this results in an -EINVAL return from
> do_splice_from[4].
> 
> This essentially means that sendfile will not work for any binary attribute
> in the sysfs.

Have you tried fixing this with a patch much like what we did for the
proc files that needed this?  If not, can you?

> [1]: https://github.com/torvalds/linux/commit/36e2c7421f02a22f71c9283e55fdb672a9eb58e7#diff-70c49af2ed5805fc1406ed6e6532d6a029ada1abd90cca6442711b9cecd4d523
> [2]: https://github.com/torvalds/linux/blob/master/drivers/base/firmware_loader/main.c#L831
> [3]: https://github.com/torvalds/linux/blob/master/fs/read_write.c#L1257
> [4]: https://github.com/torvalds/linux/blob/master/fs/splice.c#L753

kernel development is on git.kernel.org, not github :)

thanks,

greg k-h

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Siddharth Gupta]

On 1/4/2021 10:36 PM, Greg KH wrote:
> On Mon, Jan 04, 2021 at 02:43:45PM -0800, Siddharth Gupta wrote:
>> Hi all,
>>
>> With the introduction of the filesystem change "fs: don't allow splice
>> read/write without explicit ops"[1] the fallback mechanism of the firmware
>> loader[2] no longer works when using sendfile[3] from the userspace.
> What userspace program are you using to load firmware?
The userspace program is in the android userspace which listens to a 
uevent from the firmware loader and then loads the firmware using 
sendfile[1].
>   Are you not using the in-kernel firmware loader for some reason?
We have certain non-standard firmware paths that should not be added to 
the linux kernel, and the firmware_class.path only supports a single path.
>
>> Since the binary attributes don't support splice_{read,write} functions the
>> calls to splice_{read,write} used the default kernel_{read,write} functions.
>> With the above change this results in an -EINVAL return from
>> do_splice_from[4].
>>
>> This essentially means that sendfile will not work for any binary attribute
>> in the sysfs.
> Have you tried fixing this with a patch much like what we did for the
> proc files that needed this?  If not, can you?
I am not aware of this fix, could you provide me a link for reference? I 
will try it out.
>
>> [1]: https://github.com/torvalds/linux/commit/36e2c7421f02a22f71c9283e55fdb672a9eb58e7#diff-70c49af2ed5805fc1406ed6e6532d6a029ada1abd90cca6442711b9cecd4d523
>> [2]: https://github.com/torvalds/linux/blob/master/drivers/base/firmware_loader/main.c#L831
>> [3]: https://github.com/torvalds/linux/blob/master/fs/read_write.c#L1257
>> [4]: https://github.com/torvalds/linux/blob/master/fs/splice.c#L753
> kernel development is on git.kernel.org, not github :)
I use it because it is easier on the eyes when looking at diffs :D
But I'll be sure to use git.kernel.org from now on if that is what is 
preferred!
>
> thanks,
>
> greg k-h
Thanks,
Sid

[1]: 
https://android.googlesource.com/platform/system/core/+/refs/heads/master/init/firmware_handler.cpp#55

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Greg KH]

On Tue, Jan 05, 2021 at 05:00:58PM -0800, Siddharth Gupta wrote:
> 
> On 1/4/2021 10:36 PM, Greg KH wrote:
> > On Mon, Jan 04, 2021 at 02:43:45PM -0800, Siddharth Gupta wrote:
> > > Hi all,
> > > 
> > > With the introduction of the filesystem change "fs: don't allow splice
> > > read/write without explicit ops"[1] the fallback mechanism of the firmware
> > > loader[2] no longer works when using sendfile[3] from the userspace.
> > What userspace program are you using to load firmware?
> The userspace program is in the android userspace which listens to a uevent
> from the firmware loader and then loads the firmware using sendfile[1].
> >   Are you not using the in-kernel firmware loader for some reason?
> We have certain non-standard firmware paths that should not be added to the
> linux kernel, and the firmware_class.path only supports a single path.

That option is just for a single override, which should be all that you
need if the other paths that are built into the kernel do not work.
Surely one of the 5 different paths here are acceptable?

If not, how many more do you need?

And last I looked, Android wants you to use the built-in kernel firmware
loader, and NOT an external firmware binary anymore.  So this shouldn't
be an issue for your newer systems anyway :)

> > > Since the binary attributes don't support splice_{read,write} functions the
> > > calls to splice_{read,write} used the default kernel_{read,write} functions.
> > > With the above change this results in an -EINVAL return from
> > > do_splice_from[4].
> > > 
> > > This essentially means that sendfile will not work for any binary attribute
> > > in the sysfs.
> > Have you tried fixing this with a patch much like what we did for the
> > proc files that needed this?  If not, can you?
> I am not aware of this fix, could you provide me a link for reference? I
> will try it out.

Look at the series of commits starting at fe33850ff798 ("proc: wire up
generic_file_splice_read for iter ops") for how this was fixed in procfs
as an example of what also needs to be done for binary sysfs files.

thanks,

greg k-h

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Siddharth Gupta]

On 1/6/2021 2:33 AM, Greg KH wrote:
> On Tue, Jan 05, 2021 at 05:00:58PM -0800, Siddharth Gupta wrote:
>> On 1/4/2021 10:36 PM, Greg KH wrote:
>>> On Mon, Jan 04, 2021 at 02:43:45PM -0800, Siddharth Gupta wrote:
>>>> Hi all,
>>>>
>>>> With the introduction of the filesystem change "fs: don't allow splice
>>>> read/write without explicit ops"[1] the fallback mechanism of the firmware
>>>> loader[2] no longer works when using sendfile[3] from the userspace.
>>> What userspace program are you using to load firmware?
>> The userspace program is in the android userspace which listens to a uevent
>> from the firmware loader and then loads the firmware using sendfile[1].
>>>    Are you not using the in-kernel firmware loader for some reason?
>> We have certain non-standard firmware paths that should not be added to the
>> linux kernel, and the firmware_class.path only supports a single path.
> That option is just for a single override, which should be all that you
> need if the other paths that are built into the kernel do not work.
> Surely one of the 5 different paths here are acceptable?
Unfortunately they are not, and we understand that such changes 
shouldn't make it to upstream hence it was not a part of the request. If 
the firmware loader fallback mechanism was being deprecated then we 
would have to look into our options. As of now the series of changes 
breaking the sysfs bin attributes is the only bug that affects us.
>
> If not, how many more do you need?
We need 2 paths.
>
> And last I looked, Android wants you to use the built-in kernel firmware
> loader, and NOT an external firmware binary anymore.  So this shouldn't
> be an issue for your newer systems anyway :)
In our discussion with the Android team that is not the case currently. 
In the future yes, but not now :)

Regardless this bug is in the kernel and not Android. If the firmware 
loader fallback mechanism is used on the current kernel we would see the 
issue with sendfile in the userspace whether Android is running or not.
>
>>>> Since the binary attributes don't support splice_{read,write} functions the
>>>> calls to splice_{read,write} used the default kernel_{read,write} functions.
>>>> With the above change this results in an -EINVAL return from
>>>> do_splice_from[4].
>>>>
>>>> This essentially means that sendfile will not work for any binary attribute
>>>> in the sysfs.
>>> Have you tried fixing this with a patch much like what we did for the
>>> proc files that needed this?  If not, can you?
>> I am not aware of this fix, could you provide me a link for reference? I
>> will try it out.
> Look at the series of commits starting at fe33850ff798 ("proc: wire up
> generic_file_splice_read for iter ops") for how this was fixed in procfs
> as an example of what also needs to be done for binary sysfs files.
I tried to follow these fixes, but I am unfamiliar with fs code. I don't 
see the generic_file_splice_write function anymore on newer kernels, 
also AFAICT kernfs_ops does not define {read,write}_iter operations. If 
the solution is simple and someone could provide the patches I would be 
happy to test them out. If not, some more information about how to 
proceed would be nice.

Thanks,
Sid
>
> thanks,
>
> greg k-h

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Greg KH]

On Thu, Jan 07, 2021 at 02:03:47PM -0800, Siddharth Gupta wrote:
> On 1/6/2021 2:33 AM, Greg KH wrote:
> > > > > Since the binary attributes don't support splice_{read,write} functions the
> > > > > calls to splice_{read,write} used the default kernel_{read,write} functions.
> > > > > With the above change this results in an -EINVAL return from
> > > > > do_splice_from[4].
> > > > > 
> > > > > This essentially means that sendfile will not work for any binary attribute
> > > > > in the sysfs.
> > > > Have you tried fixing this with a patch much like what we did for the
> > > > proc files that needed this?  If not, can you?
> > > I am not aware of this fix, could you provide me a link for reference? I
> > > will try it out.
> > Look at the series of commits starting at fe33850ff798 ("proc: wire up
> > generic_file_splice_read for iter ops") for how this was fixed in procfs
> > as an example of what also needs to be done for binary sysfs files.
> I tried to follow these fixes, but I am unfamiliar with fs code. I don't see
> the generic_file_splice_write function anymore on newer kernels, also AFAICT
> kernfs_ops does not define {read,write}_iter operations. If the solution is
> simple and someone could provide the patches I would be happy to test them
> out. If not, some more information about how to proceed would be nice.

Can you try this tiny patch out below?

thanks,

greg k-h

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index f277d023ebcd..113bc816d430 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -968,6 +968,8 @@ const struct file_operations kernfs_file_fops = {
 	.release	= kernfs_fop_release,
 	.poll		= kernfs_fop_poll,
 	.fsync		= noop_fsync,
+	.splice_read	= generic_file_splice_read,
+	.splice_write	= iter_file_splice_write,
 };
 
 /**

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Siddharth Gupta]

On 1/8/2021 6:44 AM, Greg KH wrote:
> On Thu, Jan 07, 2021 at 02:03:47PM -0800, Siddharth Gupta wrote:
>> On 1/6/2021 2:33 AM, Greg KH wrote:
>>>>>> Since the binary attributes don't support splice_{read,write} functions the
>>>>>> calls to splice_{read,write} used the default kernel_{read,write} functions.
>>>>>> With the above change this results in an -EINVAL return from
>>>>>> do_splice_from[4].
>>>>>>
>>>>>> This essentially means that sendfile will not work for any binary attribute
>>>>>> in the sysfs.
>>>>> Have you tried fixing this with a patch much like what we did for the
>>>>> proc files that needed this?  If not, can you?
>>>> I am not aware of this fix, could you provide me a link for reference? I
>>>> will try it out.
>>> Look at the series of commits starting at fe33850ff798 ("proc: wire up
>>> generic_file_splice_read for iter ops") for how this was fixed in procfs
>>> as an example of what also needs to be done for binary sysfs files.
>> I tried to follow these fixes, but I am unfamiliar with fs code. I don't see
>> the generic_file_splice_write function anymore on newer kernels, also AFAICT
>> kernfs_ops does not define {read,write}_iter operations. If the solution is
>> simple and someone could provide the patches I would be happy to test them
>> out. If not, some more information about how to proceed would be nice.
> Can you try this tiny patch out below?
Sorry for the delay, I tried out the patch, but I am still seeing the 
error. Please take a look at these logs with
android running in the userspace[1]:

[   62.295056][  T249] remoteproc remoteproc1: powering up 
xxxxxxxx.remoteproc-cdsp
[   62.304138][  T249] remoteproc remoteproc1: Direct firmware load for 
cdsp.mdt failed with error -2
[   62.312976][  T249] remoteproc remoteproc1: Falling back to sysfs 
fallback for: cdsp.mdt
[   62.469748][  T394] ueventd: firmware: loading 'cdsp.mdt' for 
'/devices/platform/soc/xxxxxxxx.remoteproc-cdsp/remoteproc/remoteproc1/cdsp.mdt'
[   62.498700][  T394] ueventd: firmware: sendfile failed { 
'/sys/devices/platform/soc/xxxxxxxx.remoteproc-cdsp/remoteproc/remoteproc1/cdsp.mdt', 
'cdsp.mdt' }: Invalid argument

Thanks,
Sid

[1]: 
https://android.googlesource.com/platform/system/core/+/refs/heads/master/init/firmware_handler.cpp#57
>
> thanks,
>
> greg k-h
>
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index f277d023ebcd..113bc816d430 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -968,6 +968,8 @@ const struct file_operations kernfs_file_fops = {
>   	.release	= kernfs_fop_release,
>   	.poll		= kernfs_fop_poll,
>   	.fsync		= noop_fsync,
> +	.splice_read	= generic_file_splice_read,
> +	.splice_write	= iter_file_splice_write,
>   };
>   
>   /**

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Greg KH]

On Tue, Jan 12, 2021 at 10:31:26AM -0800, Siddharth Gupta wrote:
> 
> On 1/8/2021 6:44 AM, Greg KH wrote:
> > On Thu, Jan 07, 2021 at 02:03:47PM -0800, Siddharth Gupta wrote:
> > > On 1/6/2021 2:33 AM, Greg KH wrote:
> > > > > > > Since the binary attributes don't support splice_{read,write} functions the
> > > > > > > calls to splice_{read,write} used the default kernel_{read,write} functions.
> > > > > > > With the above change this results in an -EINVAL return from
> > > > > > > do_splice_from[4].
> > > > > > > 
> > > > > > > This essentially means that sendfile will not work for any binary attribute
> > > > > > > in the sysfs.
> > > > > > Have you tried fixing this with a patch much like what we did for the
> > > > > > proc files that needed this?  If not, can you?
> > > > > I am not aware of this fix, could you provide me a link for reference? I
> > > > > will try it out.
> > > > Look at the series of commits starting at fe33850ff798 ("proc: wire up
> > > > generic_file_splice_read for iter ops") for how this was fixed in procfs
> > > > as an example of what also needs to be done for binary sysfs files.
> > > I tried to follow these fixes, but I am unfamiliar with fs code. I don't see
> > > the generic_file_splice_write function anymore on newer kernels, also AFAICT
> > > kernfs_ops does not define {read,write}_iter operations. If the solution is
> > > simple and someone could provide the patches I would be happy to test them
> > > out. If not, some more information about how to proceed would be nice.
> > Can you try this tiny patch out below?
> Sorry for the delay, I tried out the patch, but I am still seeing the error.
> Please take a look at these logs with
> android running in the userspace[1]:
> 
> [   62.295056][  T249] remoteproc remoteproc1: powering up
> xxxxxxxx.remoteproc-cdsp
> [   62.304138][  T249] remoteproc remoteproc1: Direct firmware load for
> cdsp.mdt failed with error -2
> [   62.312976][  T249] remoteproc remoteproc1: Falling back to sysfs
> fallback for: cdsp.mdt
> [   62.469748][  T394] ueventd: firmware: loading 'cdsp.mdt' for '/devices/platform/soc/xxxxxxxx.remoteproc-cdsp/remoteproc/remoteproc1/cdsp.mdt'
> [   62.498700][  T394] ueventd: firmware: sendfile failed { '/sys/devices/platform/soc/xxxxxxxx.remoteproc-cdsp/remoteproc/remoteproc1/cdsp.mdt',
> 'cdsp.mdt' }: Invalid argument
> 
> Thanks,
> Sid
> 
> [1]: https://android.googlesource.com/platform/system/core/+/refs/heads/master/init/firmware_handler.cpp#57

Thanks for letting me know.  I'll try to work on this on Monday and add
splice to the in-kernel firmware testing suite, as it would have caught
this...

greg k-h

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Siddharth Gupta]

On 1/15/2021 8:20 AM, Greg KH wrote:
> On Tue, Jan 12, 2021 at 10:31:26AM -0800, Siddharth Gupta wrote:
>> On 1/8/2021 6:44 AM, Greg KH wrote:
>>> On Thu, Jan 07, 2021 at 02:03:47PM -0800, Siddharth Gupta wrote:
>>>> On 1/6/2021 2:33 AM, Greg KH wrote:
>>>>>>>> Since the binary attributes don't support splice_{read,write} functions the
>>>>>>>> calls to splice_{read,write} used the default kernel_{read,write} functions.
>>>>>>>> With the above change this results in an -EINVAL return from
>>>>>>>> do_splice_from[4].
>>>>>>>>
>>>>>>>> This essentially means that sendfile will not work for any binary attribute
>>>>>>>> in the sysfs.
>>>>>>> Have you tried fixing this with a patch much like what we did for the
>>>>>>> proc files that needed this?  If not, can you?
>>>>>> I am not aware of this fix, could you provide me a link for reference? I
>>>>>> will try it out.
>>>>> Look at the series of commits starting at fe33850ff798 ("proc: wire up
>>>>> generic_file_splice_read for iter ops") for how this was fixed in procfs
>>>>> as an example of what also needs to be done for binary sysfs files.
>>>> I tried to follow these fixes, but I am unfamiliar with fs code. I don't see
>>>> the generic_file_splice_write function anymore on newer kernels, also AFAICT
>>>> kernfs_ops does not define {read,write}_iter operations. If the solution is
>>>> simple and someone could provide the patches I would be happy to test them
>>>> out. If not, some more information about how to proceed would be nice.
>>> Can you try this tiny patch out below?
>> Sorry for the delay, I tried out the patch, but I am still seeing the error.
>> Please take a look at these logs with
>> android running in the userspace[1]:
>>
>> [   62.295056][  T249] remoteproc remoteproc1: powering up
>> xxxxxxxx.remoteproc-cdsp
>> [   62.304138][  T249] remoteproc remoteproc1: Direct firmware load for
>> cdsp.mdt failed with error -2
>> [   62.312976][  T249] remoteproc remoteproc1: Falling back to sysfs
>> fallback for: cdsp.mdt
>> [   62.469748][  T394] ueventd: firmware: loading 'cdsp.mdt' for '/devices/platform/soc/xxxxxxxx.remoteproc-cdsp/remoteproc/remoteproc1/cdsp.mdt'
>> [   62.498700][  T394] ueventd: firmware: sendfile failed { '/sys/devices/platform/soc/xxxxxxxx.remoteproc-cdsp/remoteproc/remoteproc1/cdsp.mdt',
>> 'cdsp.mdt' }: Invalid argument
>>
>> Thanks,
>> Sid
>>
>> [1]: https://android.googlesource.com/platform/system/core/+/refs/heads/master/init/firmware_handler.cpp#57
> Thanks for letting me know.  I'll try to work on this on Monday and add
> splice to the in-kernel firmware testing suite, as it would have caught
> this...
If my understanding is correct these default (or generic if you prefer)
functions require the write_iter[1] and read_iter[2] file ops to be 
defined for
kernfs_file_fops. Are there any similar default {write,read}_iter 
functions that
I can use, or do these need to be thin wrappers for the read function[3] 
for bin
attributes itself?

Thanks,
Sid

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/splice.c#n686
[2]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/splice.c#n311
[3]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/sysfs/file.c#n235
> greg k-h

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Christoph Hellwig]

Can you give this patch a spin?


diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index f277d023ebcd14..4b5833b3059f9c 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -14,6 +14,7 @@
 #include <linux/pagemap.h>
 #include <linux/sched/mm.h>
 #include <linux/fsnotify.h>
+#include <linux/uio.h>
 
 #include "kernfs-internal.h"
 
@@ -180,11 +181,10 @@ static const struct seq_operations kernfs_seq_ops = {
  * it difficult to use seq_file.  Implement simplistic custom buffering for
  * bin files.
  */
-static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
-				       char __user *user_buf, size_t count,
-				       loff_t *ppos)
+static ssize_t kernfs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 {
-	ssize_t len = min_t(size_t, count, PAGE_SIZE);
+	struct kernfs_open_file *of = kernfs_of(iocb->ki_filp);
+	ssize_t len = min_t(size_t, iov_iter_count(iter), PAGE_SIZE);
 	const struct kernfs_ops *ops;
 	char *buf;
 
@@ -210,7 +210,7 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	of->event = atomic_read(&of->kn->attr.open->event);
 	ops = kernfs_ops(of->kn);
 	if (ops->read)
-		len = ops->read(of, buf, len, *ppos);
+		len = ops->read(of, buf, len, iocb->ki_pos);
 	else
 		len = -EINVAL;
 
@@ -220,12 +220,12 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	if (len < 0)
 		goto out_free;
 
-	if (copy_to_user(user_buf, buf, len)) {
+	if (copy_to_iter(buf, len, iter) != len) {
 		len = -EFAULT;
 		goto out_free;
 	}
 
-	*ppos += len;
+	iocb->ki_pos += len;
 
  out_free:
 	if (buf == of->prealloc_buf)
@@ -235,31 +235,14 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
 	return len;
 }
 
-/**
- * kernfs_fop_read - kernfs vfs read callback
- * @file: file pointer
- * @user_buf: data to write
- * @count: number of bytes
- * @ppos: starting offset
- */
-static ssize_t kernfs_fop_read(struct file *file, char __user *user_buf,
-			       size_t count, loff_t *ppos)
+static ssize_t kernfs_fop_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 {
-	struct kernfs_open_file *of = kernfs_of(file);
-
-	if (of->kn->flags & KERNFS_HAS_SEQ_SHOW)
-		return seq_read(file, user_buf, count, ppos);
-	else
-		return kernfs_file_direct_read(of, user_buf, count, ppos);
+	if (kernfs_of(iocb->ki_filp)->kn->flags & KERNFS_HAS_SEQ_SHOW)
+		return seq_read_iter(iocb, iter);
+	return kernfs_file_read_iter(iocb, iter);
 }
 
-/**
- * kernfs_fop_write - kernfs vfs write callback
- * @file: file pointer
- * @user_buf: data to write
- * @count: number of bytes
- * @ppos: starting offset
- *
+/*
  * Copy data in from userland and pass it to the matching kernfs write
  * operation.
  *
@@ -269,20 +252,18 @@ static ssize_t kernfs_fop_read(struct file *file, char __user *user_buf,
  * modify only the the value you're changing, then write entire buffer
  * back.
  */
-static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
-				size_t count, loff_t *ppos)
+static ssize_t kernfs_fop_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 {
-	struct kernfs_open_file *of = kernfs_of(file);
+	struct kernfs_open_file *of = kernfs_of(iocb->ki_filp);
+	ssize_t len = iov_iter_count(iter);
 	const struct kernfs_ops *ops;
-	ssize_t len;
 	char *buf;
 
 	if (of->atomic_write_len) {
-		len = count;
 		if (len > of->atomic_write_len)
 			return -E2BIG;
 	} else {
-		len = min_t(size_t, count, PAGE_SIZE);
+		len = min_t(size_t, len, PAGE_SIZE);
 	}
 
 	buf = of->prealloc_buf;
@@ -293,7 +274,7 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
 	if (!buf)
 		return -ENOMEM;
 
-	if (copy_from_user(buf, user_buf, len)) {
+	if (copy_from_iter(buf, len, iter) != len) {
 		len = -EFAULT;
 		goto out_free;
 	}
@@ -312,7 +293,7 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
 
 	ops = kernfs_ops(of->kn);
 	if (ops->write)
-		len = ops->write(of, buf, len, *ppos);
+		len = ops->write(of, buf, len, iocb->ki_pos);
 	else
 		len = -EINVAL;
 
@@ -320,7 +301,7 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
 	mutex_unlock(&of->mutex);
 
 	if (len > 0)
-		*ppos += len;
+		iocb->ki_pos += len;
 
 out_free:
 	if (buf == of->prealloc_buf)
@@ -960,14 +941,16 @@ void kernfs_notify(struct kernfs_node *kn)
 EXPORT_SYMBOL_GPL(kernfs_notify);
 
 const struct file_operations kernfs_file_fops = {
-	.read		= kernfs_fop_read,
-	.write		= kernfs_fop_write,
+	.read_iter	= kernfs_fop_read_iter,
+	.write_iter	= kernfs_fop_write_iter,
 	.llseek		= generic_file_llseek,
 	.mmap		= kernfs_fop_mmap,
 	.open		= kernfs_fop_open,
 	.release	= kernfs_fop_release,
 	.poll		= kernfs_fop_poll,
 	.fsync		= noop_fsync,
+	.splice_read	= generic_file_splice_read,
+	.splice_write	= iter_file_splice_write,
 };
 
 /**

Re: PROBLEM: Firmware loader fallback mechanism no longer works with sendfile

[Siddharth Gupta]

On 1/20/2021 1:10 AM, Christoph Hellwig wrote:
> Can you give this patch a spin?
Thanks! This patch fixed the fallback mechanism for me. Attaching logs:

[   84.410162][  T244] qcom_q6v5_pas xxxxxxxx.remoteproc-cdsp: Direct 
firmware load for cdsp.bX failed with error -2
[   84.418276][  T244] qcom_q6v5_pas xxxxxxxx.remoteproc-cdsp: Falling 
back to sysfs fallback for: cdsp.bX
[   84.471558][  T393] ueventd: firmware: loading 'cdsp.bX' for 
'/devices/platform/soc/xxxxxxxx.remoteproc-cdsp/firmware/cdsp.bX'
[   84.491936][  T393] ueventd: loading 
/devices/platform/soc/xxxxxxxx.remoteproc-cdsp/firmware/cdsp.bX took 22ms
[  103.331486][  T244] remoteproc remoteproc1: remote processor 
xxxxxxxx.remoteproc-cdsp is now up


- Sid
>
>
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index f277d023ebcd14..4b5833b3059f9c 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -14,6 +14,7 @@
>   #include <linux/pagemap.h>
>   #include <linux/sched/mm.h>
>   #include <linux/fsnotify.h>
> +#include <linux/uio.h>
>   
>   #include "kernfs-internal.h"
>   
> @@ -180,11 +181,10 @@ static const struct seq_operations kernfs_seq_ops = {
>    * it difficult to use seq_file.  Implement simplistic custom buffering for
>    * bin files.
>    */
> -static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
> -				       char __user *user_buf, size_t count,
> -				       loff_t *ppos)
> +static ssize_t kernfs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>   {
> -	ssize_t len = min_t(size_t, count, PAGE_SIZE);
> +	struct kernfs_open_file *of = kernfs_of(iocb->ki_filp);
> +	ssize_t len = min_t(size_t, iov_iter_count(iter), PAGE_SIZE);
>   	const struct kernfs_ops *ops;
>   	char *buf;
>   
> @@ -210,7 +210,7 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
>   	of->event = atomic_read(&of->kn->attr.open->event);
>   	ops = kernfs_ops(of->kn);
>   	if (ops->read)
> -		len = ops->read(of, buf, len, *ppos);
> +		len = ops->read(of, buf, len, iocb->ki_pos);
>   	else
>   		len = -EINVAL;
>   
> @@ -220,12 +220,12 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
>   	if (len < 0)
>   		goto out_free;
>   
> -	if (copy_to_user(user_buf, buf, len)) {
> +	if (copy_to_iter(buf, len, iter) != len) {
>   		len = -EFAULT;
>   		goto out_free;
>   	}
>   
> -	*ppos += len;
> +	iocb->ki_pos += len;
>   
>    out_free:
>   	if (buf == of->prealloc_buf)
> @@ -235,31 +235,14 @@ static ssize_t kernfs_file_direct_read(struct kernfs_open_file *of,
>   	return len;
>   }
>   
> -/**
> - * kernfs_fop_read - kernfs vfs read callback
> - * @file: file pointer
> - * @user_buf: data to write
> - * @count: number of bytes
> - * @ppos: starting offset
> - */
> -static ssize_t kernfs_fop_read(struct file *file, char __user *user_buf,
> -			       size_t count, loff_t *ppos)
> +static ssize_t kernfs_fop_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>   {
> -	struct kernfs_open_file *of = kernfs_of(file);
> -
> -	if (of->kn->flags & KERNFS_HAS_SEQ_SHOW)
> -		return seq_read(file, user_buf, count, ppos);
> -	else
> -		return kernfs_file_direct_read(of, user_buf, count, ppos);
> +	if (kernfs_of(iocb->ki_filp)->kn->flags & KERNFS_HAS_SEQ_SHOW)
> +		return seq_read_iter(iocb, iter);
> +	return kernfs_file_read_iter(iocb, iter);
>   }
>   
> -/**
> - * kernfs_fop_write - kernfs vfs write callback
> - * @file: file pointer
> - * @user_buf: data to write
> - * @count: number of bytes
> - * @ppos: starting offset
> - *
> +/*
>    * Copy data in from userland and pass it to the matching kernfs write
>    * operation.
>    *
> @@ -269,20 +252,18 @@ static ssize_t kernfs_fop_read(struct file *file, char __user *user_buf,
>    * modify only the the value you're changing, then write entire buffer
>    * back.
>    */
> -static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
> -				size_t count, loff_t *ppos)
> +static ssize_t kernfs_fop_write_iter(struct kiocb *iocb, struct iov_iter *iter)
>   {
> -	struct kernfs_open_file *of = kernfs_of(file);
> +	struct kernfs_open_file *of = kernfs_of(iocb->ki_filp);
> +	ssize_t len = iov_iter_count(iter);
>   	const struct kernfs_ops *ops;
> -	ssize_t len;
>   	char *buf;
>   
>   	if (of->atomic_write_len) {
> -		len = count;
>   		if (len > of->atomic_write_len)
>   			return -E2BIG;
>   	} else {
> -		len = min_t(size_t, count, PAGE_SIZE);
> +		len = min_t(size_t, len, PAGE_SIZE);
>   	}
>   
>   	buf = of->prealloc_buf;
> @@ -293,7 +274,7 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
>   	if (!buf)
>   		return -ENOMEM;
>   
> -	if (copy_from_user(buf, user_buf, len)) {
> +	if (copy_from_iter(buf, len, iter) != len) {
>   		len = -EFAULT;
>   		goto out_free;
>   	}
> @@ -312,7 +293,7 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
>   
>   	ops = kernfs_ops(of->kn);
>   	if (ops->write)
> -		len = ops->write(of, buf, len, *ppos);
> +		len = ops->write(of, buf, len, iocb->ki_pos);
>   	else
>   		len = -EINVAL;
>   
> @@ -320,7 +301,7 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
>   	mutex_unlock(&of->mutex);
>   
>   	if (len > 0)
> -		*ppos += len;
> +		iocb->ki_pos += len;
>   
>   out_free:
>   	if (buf == of->prealloc_buf)
> @@ -960,14 +941,16 @@ void kernfs_notify(struct kernfs_node *kn)
>   EXPORT_SYMBOL_GPL(kernfs_notify);
>   
>   const struct file_operations kernfs_file_fops = {
> -	.read		= kernfs_fop_read,
> -	.write		= kernfs_fop_write,
> +	.read_iter	= kernfs_fop_read_iter,
> +	.write_iter	= kernfs_fop_write_iter,
>   	.llseek		= generic_file_llseek,
>   	.mmap		= kernfs_fop_mmap,
>   	.open		= kernfs_fop_open,
>   	.release	= kernfs_fop_release,
>   	.poll		= kernfs_fop_poll,
>   	.fsync		= noop_fsync,
> +	.splice_read	= generic_file_splice_read,
> +	.splice_write	= iter_file_splice_write,
>   };
>   
>   /**