Re: [PATCH] virtiofs: include a newline in sysfs tag

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2570 bytes --]

On Mon, May 06, 2024 at 02:57:18PM -0400, Brian Foster wrote:
> On Tue, Apr 30, 2024 at 01:34:31PM -0400, Stefan Hajnoczi wrote:
> > On Thu, Apr 25, 2024 at 06:44:00AM -0400, Brian Foster wrote:
> > > The internal tag string doesn't contain a newline. Append one when
> > > emitting the tag via sysfs.
> > > 
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > ---
> > > 
> > > Hi all,
> > > 
> > > I just noticed this and it seemed a little odd to me compared to typical
> > > sysfs output, but maybe it was intentional..? Easy enough to send a
> > > patch either way.. thoughts?
> > 
> > Hi Brian,
> > Orthogonal to the newline issue, sysfs_emit(buf, "%s", fs->tag) is
> > needed to prevent format string injection. Please mention this in the
> > commit description. I'm afraid I introduced that bug, sorry!
> > 
> 
> Hi Stefan,
> 
> Ah, thanks. That hadn't crossed my mind.
> 
> > Regarding newline, I'm concerned that adding a newline might break
> > existing programs. Unless there is a concrete need to have the newline,
> > I would keep things as they are.
> > 
> 
> Not sure I follow the concern.. wasn't this interface just added? Did
> you have certain userspace tools in mind?

v6.9-rc7 has already been tagged and might be the last tag (I'm not
sure). If v6.9 is released without the newline, then changing it in the
next kernel release could cause breakage. Some ideas on how userspace
might break:

- Userspace calls mount(2) with the contents of the sysfs attr as the
  source (i.e. "myfs\n" vs "myfs").

- Userspace stores the contents of the sysfs attr in a file and runs
  again later on a new kernel after the format has changed, causing tag
  comparisons to fail.

> FWIW, my reason for posting this was that the first thing I did to try
> out this functionality was basically a 'cat /sys/fs/virtiofs/*/tag' to
> see what fs' were attached to my vm, and then I got a single line
> concatenation of every virtiofs tag and found that pretty annoying. ;)

Understood.

> I don't know that is a concrete need for the newline, but I still find
> the current behavior kind of odd. That said, I'll defer to you guys if
> you'd prefer to leave it alone. I just posted a v2 for the format
> specifier thing as above and you can decide which patch to take or not..

The v6.9 release will happen soon and I'm not sure if we can still get
the patch in. I've asked Miklos if your patch can be merged with the
newline added for v6.9. That would solve the userspace breakage
concerns.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]