[PATCH] virtiofs: include a newline in sysfs tag

[PATCH] virtiofs: include a newline in sysfs tag

[Brian Foster]

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?

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

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

[Vivek Goyal]

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?

In my initial patch I had added a newline char. Then someone gave examples
where sysfs output did not have newline char. So I got rid of it. After
that Stefan posted a new patch series that did not include newline. So
yes it was intentional.

I am sitting on the fence on this one. Don't have a strong preference
either way. Others might have good arguments one way or the other.

Thanks
Vivek

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

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

[Stefan Hajnoczi]

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

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!

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.

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
> 

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

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

[Brian Foster]

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

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

[Stefan Hajnoczi]

[-- 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 --]

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

[Brian Foster]

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

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

[Miklos Szeredi]

On Tue, 7 May 2024 at 16:03, Stefan Hajnoczi <stefanha@redhat.com> wrote:

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

Will do.  There was already a fix queued for 6.9 waiting for a companion.

Thanks,
Miklos