Re: [PATCH] sysfs: clamp show() return value in sysfs_kf_read()

[David Laight <david.laight.linux@gmail.com>]

On Thu, 21 May 2026 06:18:09 -1000
Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> On Thu, May 21, 2026 at 08:18:32AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, May 20, 2026 at 08:19:34AM -1000, Tejun Heo wrote:  
> > > Hello,
> > > 
> > > Two nits:
> > > 
> > > - Buffer is atomic_write_len ?: PAGE_SIZE, so probably better to clamp
> > >   to that than hardcode PAGE_SIZE.  
> > 
> > Where is that check at?  And sysfs_kf_seq_show() doesn't check it this
> > way, should that change?  
> 
> In kernfs_fop_open(), if ops->prealloc, we allocate of->prealloc_buf to the
> size of of->atomic_write_len ?: PAGE_SIZE. Maybe we should just update
> of->atomic_write_len. I think the intention was to allow >PAGE_SIZE atomic
> content. seq_file now does dynamic buffer resizing, so this may not be
> necessary. There's no clean interface to preemptly set the minimum buffer
> size right now but that probably is the better direction. Then, we can just
> always go through seq_file.

Except that the buffer size is always PAGE_SIZE in the prealloc paths.
A larger size is used by the cgroup code - but that is really for writes.

There seems to have been a lot of rework to all this code around 3.13.
I can't quite see what it looked like before.

But I think the write code should reject writes longer than
of->atomic_write_len ?: PAGE_SIZE because all writes are treated as
having 'offset zero'.
If op->atomic_write_len is non-zero overlong writes are rejected.
If op->atomic_write_len is zero then writes are truncated and userspace
is likely to do a second write for the remainder - which won't DTRT.
The naming suggests that loop existed in the kernel at some point,
but I can't find that version.

The 'prealloc' code was added later and reused 'atomic_write_len' for
the buffer size. But the only users leave it as default - allowing
PAGE_SIZE requests which require a kmalloc(PAGE_SIZE + 1) (aka 2 pages)
because of the trailing '\0' added to writes.

The 'prealloc' code is all about trying to avoid a kmalloc() that might
fail and cause a deadlock.
But I suspect the daemon doing the system call wont actually get to that
kmalloc() if memory to so short it fails to allocate the IO buffer.
Even if that were a problem an option to make that path 'try harder'
to kmalloc() the buffer would be better than preallocating pairs of pages
for each of the six? cgroup 'files'.
(It could even fall back to a single preallocated buffer for all users
that deem it important.)

Reading this code is hard because of the tangled mess between kernfs/file.c
and sysfs/file.c.

-- David

> 
> Thanks.
>