| Message ID | 20240425104400.30222-1-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofs: include a newline in sysfs tag | expand |
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 >
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 >
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 > >
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
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
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
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);
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(-)