drivers/iio/industrialio-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
If the *ppos value is non-zero then simple_write_to_buffer() will not
initialize the start of the buf[] buffer. Non-zero ppos values aren't
going to work at all. Check for that at the start of the function and
return -EINVAL.
Fixes: cdf01e0809a4 ("iio: backend: add debugFs interface")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
drivers/iio/industrialio-backend.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index 138ebebc9c0d..4763e224ebc6 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -156,7 +156,7 @@ static ssize_t iio_backend_debugfs_write_reg(struct file *file,
ssize_t rc;
int ret;
- if (count >= sizeof(buf))
+ if (*ppos != 0 || count >= sizeof(buf))
return -ENOSPC;
rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
--
2.53.0On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
>
> If the *ppos value is non-zero then simple_write_to_buffer() will not
> initialize the start of the buf[] buffer. Non-zero ppos values aren't
> going to work at all. Check for that at the start of the function and
> return -EINVAL.
>
Commit message is incorrect, it looks like you're returning -ENOSPC here...
>
> Fixes: cdf01e0809a4 ("iio: backend: add debugFs interface")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> drivers/iio/industrialio-backend.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
> index 138ebebc9c0d..4763e224ebc6 100644
> --- a/drivers/iio/industrialio-backend.c
> +++ b/drivers/iio/industrialio-backend.c
> @@ -156,7 +156,7 @@ static ssize_t iio_backend_debugfs_write_reg(struct file *file,
> ssize_t rc;
> int ret;
>
> - if (count >= sizeof(buf))
> + if (*ppos != 0 || count >= sizeof(buf))
> return -ENOSPC;
>
See above comment.
best regards,
max
>
> rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
> --
> 2.53.0
>
>
On Mon, 25 May 2026 08:20:31 -0500
Maxwell Doose <m32285159@gmail.com> wrote:
> On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
> >
> > If the *ppos value is non-zero then simple_write_to_buffer() will not
> > initialize the start of the buf[] buffer. Non-zero ppos values aren't
> > going to work at all. Check for that at the start of the function and
> > return -EINVAL.
> >
>
> Commit message is incorrect, it looks like you're returning -ENOSPC here...
Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable.
thanks,
J
>
> >
> > Fixes: cdf01e0809a4 ("iio: backend: add debugFs interface")
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > ---
> > drivers/iio/industrialio-backend.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
> > index 138ebebc9c0d..4763e224ebc6 100644
> > --- a/drivers/iio/industrialio-backend.c
> > +++ b/drivers/iio/industrialio-backend.c
> > @@ -156,7 +156,7 @@ static ssize_t iio_backend_debugfs_write_reg(struct file *file,
> > ssize_t rc;
> > int ret;
> >
> > - if (count >= sizeof(buf))
> > + if (*ppos != 0 || count >= sizeof(buf))
> > return -ENOSPC;
> >
>
> See above comment.
>
> best regards,
> max
>
>
> >
> > rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
> > --
> > 2.53.0
> >
> >
On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote: > On Mon, 25 May 2026 08:20:31 -0500 > Maxwell Doose <m32285159@gmail.com> wrote: > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote: > > > > > > If the *ppos value is non-zero then simple_write_to_buffer() will not > > > initialize the start of the buf[] buffer. Non-zero ppos values aren't > > > going to work at all. Check for that at the start of the function and > > > return -EINVAL. > > > > Commit message is incorrect, it looks like you're returning -ENOSPC here... > Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable. I was stumbled over these patches by Dan. Since the file_operations for the code in question do not define .llseek, the seek is not supported and will return -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you elaborate on the case where the *ppos is not 0, please? -- With Best Regards, Andy Shevchenko
On Thu, Jun 04, 2026 at 10:30:34AM +0300, Andy Shevchenko wrote: > On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote: > > On Mon, 25 May 2026 08:20:31 -0500 > > Maxwell Doose <m32285159@gmail.com> wrote: > > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote: > > > > > > > > If the *ppos value is non-zero then simple_write_to_buffer() will not > > > > initialize the start of the buf[] buffer. Non-zero ppos values aren't > > > > going to work at all. Check for that at the start of the function and > > > > return -EINVAL. > > > > > > Commit message is incorrect, it looks like you're returning -ENOSPC here... > > Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable. > > I was stumbled over these patches by Dan. Since the file_operations for the code > in question do not define .llseek, the seek is not supported and will return > -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you > elaborate on the case where the *ppos is not 0, please? The simple_write_to_buffer() will update *ppos so partial writes are supported in that way. regards, dan carpenter
On Thu, Jun 04, 2026 at 10:45:36AM +0300, Dan Carpenter wrote: > On Thu, Jun 04, 2026 at 10:30:34AM +0300, Andy Shevchenko wrote: > > On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote: > > > On Mon, 25 May 2026 08:20:31 -0500 > > > Maxwell Doose <m32285159@gmail.com> wrote: > > > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote: > > > > > > > > > > If the *ppos value is non-zero then simple_write_to_buffer() will not > > > > > initialize the start of the buf[] buffer. Non-zero ppos values aren't > > > > > going to work at all. Check for that at the start of the function and > > > > > return -EINVAL. > > > > > > > > Commit message is incorrect, it looks like you're returning -ENOSPC here... > > > Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable. > > > > I was stumbled over these patches by Dan. Since the file_operations for the code > > in question do not define .llseek, the seek is not supported and will return > > -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you > > elaborate on the case where the *ppos is not 0, please? > The simple_write_to_buffer() will update *ppos so partial writes are > supported in that way. Can you show the step-by-step scenario? I'm still fail to see how it may be happen. Is it somewhere inside the kernel loop? Which VFS function(s) is responsible for that in such a case? -- With Best Regards, Andy Shevchenko
On Thu, Jun 04, 2026 at 11:03:15AM +0300, Andy Shevchenko wrote: > On Thu, Jun 04, 2026 at 10:45:36AM +0300, Dan Carpenter wrote: > > On Thu, Jun 04, 2026 at 10:30:34AM +0300, Andy Shevchenko wrote: > > > On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote: > > > > On Mon, 25 May 2026 08:20:31 -0500 > > > > Maxwell Doose <m32285159@gmail.com> wrote: > > > > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote: > > > > > > > > > > > > If the *ppos value is non-zero then simple_write_to_buffer() will not > > > > > > initialize the start of the buf[] buffer. Non-zero ppos values aren't > > > > > > going to work at all. Check for that at the start of the function and > > > > > > return -EINVAL. > > > > > > > > > > Commit message is incorrect, it looks like you're returning -ENOSPC here... > > > > Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable. > > > > > > I was stumbled over these patches by Dan. Since the file_operations for the code > > > in question do not define .llseek, the seek is not supported and will return > > > -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you > > > elaborate on the case where the *ppos is not 0, please? > > > The simple_write_to_buffer() will update *ppos so partial writes are > > supported in that way. > > Can you show the step-by-step scenario? I'm still fail to see how it may be happen. > Is it somewhere inside the kernel loop? Which VFS function(s) is responsible for > that in such a case? Even if ppos is advanced, the simple_write_to_buffer() https://elixir.bootlin.com/linux/v7.1-rc6/source/fs/libfs.c#L1188 won't write more than available in the buffer. Is the available also being advanced somehow? -- With Best Regards, Andy Shevchenko
On Thu, Jun 04, 2026 at 11:28:39AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 04, 2026 at 11:03:15AM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 04, 2026 at 10:45:36AM +0300, Dan Carpenter wrote:
> > > On Thu, Jun 04, 2026 at 10:30:34AM +0300, Andy Shevchenko wrote:
> > > > On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote:
> > > > > On Mon, 25 May 2026 08:20:31 -0500
> > > > > Maxwell Doose <m32285159@gmail.com> wrote:
> > > > > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
> > > > > > >
> > > > > > > If the *ppos value is non-zero then simple_write_to_buffer() will not
> > > > > > > initialize the start of the buf[] buffer. Non-zero ppos values aren't
> > > > > > > going to work at all. Check for that at the start of the function and
> > > > > > > return -EINVAL.
> > > > > >
> > > > > > Commit message is incorrect, it looks like you're returning -ENOSPC here...
> > > > > Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable.
> > > >
> > > > I was stumbled over these patches by Dan. Since the file_operations for the code
> > > > in question do not define .llseek, the seek is not supported and will return
> > > > -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you
> > > > elaborate on the case where the *ppos is not 0, please?
> >
> > > The simple_write_to_buffer() will update *ppos so partial writes are
> > > supported in that way.
> >
> > Can you show the step-by-step scenario? I'm still fail to see how it may be happen.
> > Is it somewhere inside the kernel loop? Which VFS function(s) is responsible for
> > that in such a case?
>
> Even if ppos is advanced, the simple_write_to_buffer()
> https://elixir.bootlin.com/linux/v7.1-rc6/source/fs/libfs.c#L1188
> won't write more than available in the buffer. Is the available also
> being advanced somehow?
It's not a buffer overflow, it's an uninitialized data bug.
I used Google AI to create a test case but my qemu system is arm64
and the test case is in assembly so I'm not sure how useful it is.
(Also I modified the Google AI code and the test case is garbage).
My test case writes 10 bytes of data at a time. I've attached the
test debugfs kernel module.
Here is the code from the kernel:
drivers/iio/industrialio-backend.c
149 static ssize_t iio_backend_debugfs_write_reg(struct file *file,
150 const char __user *userbuf,
151 size_t count, loff_t *ppos)
152 {
153 struct iio_backend *back = file->private_data;
154 unsigned int val;
155 char buf[80];
buf is uninitialized. In my test code, I initialized it to all U
characters which stands for uninitialized.
156 ssize_t rc;
157 int ret;
158
159 if (*ppos != 0 || count >= sizeof(buf))
^^^^^^^^^^
I added this check on *ppos but imagine it's not there and *ppos is
non-zero.
160 return -ENOSPC;
161
162 rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
The simple_write_to_buffer() function is designed to support partial
writes so it leaves the first 0 to *ppos characters alone.
163 if (rc < 0)
164 return rc;
165
166 buf[rc] = '\0';
The return is the number of characters written (starting at *ppos). I'm
doing 10 character writes so it is 10. The first 10 characters are
uninitialized. In my test output you can see it prints ten U characters.
167
168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
^^^
Uninitialized variable.
169
root@test:/home/ubuntu/mnt/progs/tmp/sysfs# ./a.out
Enter a line of text to save: 123456789012345678901234567890
[ 4266.853201] pos=0 count=10
[ 4266.853451] rc=10 buf 1234567890
[ 4266.854774] pos=10 count=10
Writing to file character by character...
[ 4266.858582] rc=10 buf UUUUUUUUUU
[ 4266.858698] pos=20 count=10
[ 4266.858740] rc=10 buf UUUUUUUUUU
[ 4266.858798] pos=30 count=10
[ 4266.858834] rc=10 buf UUUUUUUUUU
[ 4266.858888] pos=40 count=10
[ 4266.858924] rc=10 buf UUUUUUUUUU
[ 4266.859015] pos=50 count=10
regards,
dan carpenter
On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote:
> On Thu, Jun 04, 2026 at 11:28:39AM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 04, 2026 at 11:03:15AM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 04, 2026 at 10:45:36AM +0300, Dan Carpenter wrote:
> > > > On Thu, Jun 04, 2026 at 10:30:34AM +0300, Andy Shevchenko wrote:
> > > > > On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote:
> > > > > > On Mon, 25 May 2026 08:20:31 -0500
> > > > > > Maxwell Doose <m32285159@gmail.com> wrote:
> > > > > > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote:
> > > > > > > >
> > > > > > > > If the *ppos value is non-zero then simple_write_to_buffer() will not
> > > > > > > > initialize the start of the buf[] buffer. Non-zero ppos values aren't
> > > > > > > > going to work at all. Check for that at the start of the function and
> > > > > > > > return -EINVAL.
> > > > > > >
> > > > > > > Commit message is incorrect, it looks like you're returning -ENOSPC here...
> > > > > > Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable.
> > > > >
> > > > > I was stumbled over these patches by Dan. Since the file_operations for the code
> > > > > in question do not define .llseek, the seek is not supported and will return
> > > > > -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you
> > > > > elaborate on the case where the *ppos is not 0, please?
> > >
> > > > The simple_write_to_buffer() will update *ppos so partial writes are
> > > > supported in that way.
> > >
> > > Can you show the step-by-step scenario? I'm still fail to see how it may be happen.
> > > Is it somewhere inside the kernel loop? Which VFS function(s) is responsible for
> > > that in such a case?
> >
> > Even if ppos is advanced, the simple_write_to_buffer()
> > https://elixir.bootlin.com/linux/v7.1-rc6/source/fs/libfs.c#L1188
> > won't write more than available in the buffer. Is the available also
> > being advanced somehow?
>
> It's not a buffer overflow, it's an uninitialized data bug.
>
> I used Google AI to create a test case but my qemu system is arm64
> and the test case is in assembly so I'm not sure how useful it is.
> (Also I modified the Google AI code and the test case is garbage).
> My test case writes 10 bytes of data at a time. I've attached the
> test debugfs kernel module.
>
> Here is the code from the kernel:
>
> drivers/iio/industrialio-backend.c
> 149 static ssize_t iio_backend_debugfs_write_reg(struct file *file,
> 150 const char __user *userbuf,
> 151 size_t count, loff_t *ppos)
> 152 {
> 153 struct iio_backend *back = file->private_data;
> 154 unsigned int val;
> 155 char buf[80];
>
> buf is uninitialized. In my test code, I initialized it to all U
> characters which stands for uninitialized.
>
> 156 ssize_t rc;
> 157 int ret;
> 158
> 159 if (*ppos != 0 || count >= sizeof(buf))
> ^^^^^^^^^^
> I added this check on *ppos but imagine it's not there and *ppos is
> non-zero.
>
> 160 return -ENOSPC;
> 161
> 162 rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
>
> The simple_write_to_buffer() function is designed to support partial
> writes so it leaves the first 0 to *ppos characters alone.
If you wrote as many bytes as required, *ppos left on the 'count', and next
writes go after that. So, why *ppos is not 0? Maybe the issue that we need
somewhere reset *ppos to 0?
> 163 if (rc < 0)
> 164 return rc;
> 165
> 166 buf[rc] = '\0';
>
> The return is the number of characters written (starting at *ppos). I'm
> doing 10 character writes so it is 10. The first 10 characters are
> uninitialized. In my test output you can see it prints ten U characters.
>
> 167
> 168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val);
> ^^^
> Uninitialized variable.
>
> 169
> # ./a.out
> Enter a line of text to save: 123456789012345678901234567890
> [ 4266.853201] pos=0 count=10
> [ 4266.853451] rc=10 buf 1234567890
> [ 4266.854774] pos=10 count=10
> Writing to file character by character...
> [ 4266.858582] rc=10 buf UUUUUUUUUU
> [ 4266.858698] pos=20 count=10
> [ 4266.858740] rc=10 buf UUUUUUUUUU
> [ 4266.858798] pos=30 count=10
> [ 4266.858834] rc=10 buf UUUUUUUUUU
> [ 4266.858888] pos=40 count=10
> [ 4266.858924] rc=10 buf UUUUUUUUUU
> [ 4266.859015] pos=50 count=10
--
With Best Regards,
Andy Shevchenko
On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote: > 168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); > ^^^ > Uninitialized variable. s/variable/data/. regards, dan carpenter
On Thu, Jun 04, 2026 at 01:42:11PM +0300, Dan Carpenter wrote: > On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote: > > 168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); > > ^^^ > > Uninitialized variable. > > s/variable/data/. With what I asked in the previous reply and what you explained there (thanks, btw!) I still think your patches are not fully correct. They will require to atomically write all or nothing. If we want support partial writes we need to go with that differently (reset ppos when we got enough or more than enough data). -- With Best Regards, Andy Shevchenko
On Thu, Jun 04, 2026 at 05:55:08PM +0300, Andy Shevchenko wrote: > On Thu, Jun 04, 2026 at 01:42:11PM +0300, Dan Carpenter wrote: > > On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote: > > > 168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); > > > ^^^ > > > Uninitialized variable. > > > > s/variable/data/. > > With what I asked in the previous reply and what you explained there > (thanks, btw!) I still think your patches are not fully correct. They > will require to atomically write all or nothing. If we want support > partial writes we need to go with that differently (reset ppos when > we got enough or more than enough data). Requiring writes to syfs and debugfs be atomic is pretty normal and works well in practice. These are very small writes. regards, dan carpenter
On Fri, Jun 05, 2026 at 09:12:38AM +0300, Dan Carpenter wrote: > On Thu, Jun 04, 2026 at 05:55:08PM +0300, Andy Shevchenko wrote: > > On Thu, Jun 04, 2026 at 01:42:11PM +0300, Dan Carpenter wrote: > > > On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote: > > > > 168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); > > > > ^^^ > > > > Uninitialized variable. > > > > > > s/variable/data/. > > > > With what I asked in the previous reply and what you explained there > > (thanks, btw!) I still think your patches are not fully correct. They > > will require to atomically write all or nothing. If we want support > > partial writes we need to go with that differently (reset ppos when > > we got enough or more than enough data). > > Requiring writes to syfs and debugfs be atomic is pretty normal and > works well in practice. These are very small writes. Perhaps. In any case your patch will break existing partial writes, right? I'm still considering that resetting ppos is the right thing to do. Just need to find where the best place is to do that. -- With Best Regards, Andy Shevchenko
On Fri, 2026-06-05 at 11:28 +0300, Andy Shevchenko wrote: > On Fri, Jun 05, 2026 at 09:12:38AM +0300, Dan Carpenter wrote: > > On Thu, Jun 04, 2026 at 05:55:08PM +0300, Andy Shevchenko wrote: > > > On Thu, Jun 04, 2026 at 01:42:11PM +0300, Dan Carpenter wrote: > > > > On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote: > > > > > 168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); > > > > > ^^^ > > > > > Uninitialized variable. > > > > > > > > s/variable/data/. > > > > > > With what I asked in the previous reply and what you explained there > > > (thanks, btw!) I still think your patches are not fully correct. They > > > will require to atomically write all or nothing. If we want support > > > partial writes we need to go with that differently (reset ppos when > > > we got enough or more than enough data). > > > > Requiring writes to syfs and debugfs be atomic is pretty normal and > > works well in practice. These are very small writes. > > Perhaps. In any case your patch will break existing partial writes, right? > I'm still considering that resetting ppos is the right thing to do. Just > need to find where the best place is to do that. I think anyone doing partial writes on a debugfs interface like this one is very unlikely but it is a fair point, yes. But can't we be more relaxed on debugfs? No userspace app should be relying on debugfs in order to work (though I know that actually happens). Anyways, this is one of those interesting edge cases and easy enough to get wrong. I guess we should either: 1. Improve simple_write_to_buffer() docs; 2. Or come up with a new simple_write_once_to_buffer() helper? - Nuno Sá
On Mon, 08 Jun 2026 21:31:14 +0100 Nuno Sá <noname.nuno@gmail.com> wrote: > On Fri, 2026-06-05 at 11:28 +0300, Andy Shevchenko wrote: > > On Fri, Jun 05, 2026 at 09:12:38AM +0300, Dan Carpenter wrote: > > > On Thu, Jun 04, 2026 at 05:55:08PM +0300, Andy Shevchenko wrote: > > > > On Thu, Jun 04, 2026 at 01:42:11PM +0300, Dan Carpenter wrote: > > > > > On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote: > > > > > > 168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); > > > > > > ^^^ > > > > > > Uninitialized variable. > > > > > > > > > > s/variable/data/. > > > > > > > > With what I asked in the previous reply and what you explained there > > > > (thanks, btw!) I still think your patches are not fully correct. They > > > > will require to atomically write all or nothing. If we want support > > > > partial writes we need to go with that differently (reset ppos when > > > > we got enough or more than enough data). > > > > > > Requiring writes to syfs and debugfs be atomic is pretty normal and > > > works well in practice. These are very small writes. > > > > Perhaps. In any case your patch will break existing partial writes, right? > > I'm still considering that resetting ppos is the right thing to do. Just > > need to find where the best place is to do that. > > I think anyone doing partial writes on a debugfs interface like this one is very > unlikely but it is a fair point, yes. But can't we be more relaxed on debugfs? No > userspace app should be relying on debugfs in order to work (though I know that > actually happens). > > Anyways, this is one of those interesting edge cases and easy enough to get wrong. I > guess we should either: > > 1. Improve simple_write_to_buffer() docs; > 2. Or come up with a new simple_write_once_to_buffer() helper? > When you say "simple_write_once_to_buffer()" do you mean a wrapper around simple_write_to_buffer() that also checks if *ppos is 0 and returns -EINVAL if *ppos isn't 0? If so I can start writing that function. Though I guess we probably want to ask Jonathan about it first since obviously he'll have his own opinions. -- best regards, max
On Fri, Jun 5, 2026 at 3:28 AM Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > On Fri, Jun 05, 2026 at 09:12:38AM +0300, Dan Carpenter wrote: > > On Thu, Jun 04, 2026 at 05:55:08PM +0300, Andy Shevchenko wrote: > > > On Thu, Jun 04, 2026 at 01:42:11PM +0300, Dan Carpenter wrote: > > > > On Thu, Jun 04, 2026 at 01:38:50PM +0300, Dan Carpenter wrote: > > > > > 168 ret = sscanf(buf, "%i %i", &back->cached_reg_addr, &val); > > > > > ^^^ > > > > > Uninitialized variable. > > > > > > > > s/variable/data/. > > > > > > With what I asked in the previous reply and what you explained there > > > (thanks, btw!) I still think your patches are not fully correct. They > > > will require to atomically write all or nothing. If we want support > > > partial writes we need to go with that differently (reset ppos when > > > we got enough or more than enough data). > > > > Requiring writes to syfs and debugfs be atomic is pretty normal and > > works well in practice. These are very small writes. > > Perhaps. In any case your patch will break existing partial writes, right? > I'm still considering that resetting ppos is the right thing to do. Just > need to find where the best place is to do that. > I'm certainly no expert on simple_write_to_buffer() but something in me tells me that writing in the middle of a buffer is a code smell. But you all seem much more informed in that regard so don't trust me :)
On Thu, Jun 04, 2026 at 10:30:40AM +0300, Andy Shevchenko wrote: > On Tue, May 26, 2026 at 07:19:46PM +0100, Jonathan Cameron wrote: > > On Mon, 25 May 2026 08:20:31 -0500 > > Maxwell Doose <m32285159@gmail.com> wrote: > > > On Mon, May 25, 2026 at 2:17 AM Dan Carpenter <error27@gmail.com> wrote: > > > > > > > > If the *ppos value is non-zero then simple_write_to_buffer() will not > > > > initialize the start of the buf[] buffer. Non-zero ppos values aren't > > > > going to work at all. Check for that at the start of the function and > > > > return -EINVAL. > > > > > > Commit message is incorrect, it looks like you're returning -ENOSPC here... > > Tweaked and applied to the fixes-togreg branch of iio.git + marked for stable. > > I was stumbled over these patches by Dan. Since the file_operations for the code > in question do not define .llseek, the seek is not supported and will return > -ESPIPE. I'm not sure why we have these patches to begin with. Dan, can you > elaborate on the case where the *ppos is not 0, please? FWIW, I also left a comment in that blog post. -- With Best Regards, Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.