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

[Brian Foster <bfoster@redhat.com>]

On Tue, May 07, 2024 at 10:03:30AM -0400, Stefan Hajnoczi wrote:
> 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.
> 

OK, fair points.

> > 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.
> 

IMO, this all seems a little overblown. If the only issue ends up
missing the release deadline, then I'd say just mark it as a Fixes:
patch for the original patch in v6.9 (probably should have done that
anyways, I guess). Odds are anybody who's going to use this will pick it
up via a stable kernel (through distros and whatnot) anyways. But again
just my .02. ;)

Brian

> Stefan