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