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

[Brian Foster <bfoster@redhat.com>]

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?

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

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

Brian

> Stefan
> 
> > 
> > Brian
> > 
> >  fs/fuse/virtio_fs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > index 322af827a232..bb3e941b9503 100644
> > --- a/fs/fuse/virtio_fs.c
> > +++ b/fs/fuse/virtio_fs.c
> > @@ -170,7 +170,7 @@ static ssize_t tag_show(struct kobject *kobj,
> >  {
> >  	struct virtio_fs *fs = container_of(kobj, struct virtio_fs, kobj);
> >  
> > -	return sysfs_emit(buf, fs->tag);
> > +	return sysfs_emit(buf, "%s\n", fs->tag);
> >  }
> >  
> >  static struct kobj_attribute virtio_fs_tag_attr = __ATTR_RO(tag);
> > -- 
> > 2.44.0
> >