Re: VOP_STRATEGY for devices and pipes
On Thu, Jul 15, 2021 at 11:00:36AM -0700, Chuck Silvers wrote: > setting an extattr on a device node in UFS is already disallowed. > ffs_setextattr() has this check: > > if (ap->a_vp->v_type == VCHR || ap->a_vp->v_type == VBLK) > return (EOPNOTSUPP); > > I have not checked other file systems to see if they have an > equivalent check. I don't think anything else does extattrs. tmpfs doesn't, for example. > I think we should continue to support extattrs on fifos for two reasons: > - extattrs are used to implement ACLs, and it is useful to be able >to set an ACL on a fifo. > - it is good to remain more compatible with FreeBSD in this area. Reasonable enough. of course it's also useful to be able to set an ACL on a device... but not now. -- David A. Holland dholl...@netbsd.org
Re: VOP_STRATEGY for devices and pipes
On Thu, Jul 15, 2021 at 06:26:55PM +0200, Rhialto wrote: > > Seems to me the best/safest thing to do for now would be to prohibit > > the extended-attr on devices and fifos. > > If I have a normal file system, and in a directory is an entry with an > inode for a fifo (or a device), and I set an extended attribute on that > inode, then surely it will get stored in the file system, and not in the > fifo (or device)? > > So if the fifo (or device) code gets involved in this, then the bug is > there, I'd think? Well... yes, in theory. The way it actually works right now is that device and fifo vnodes belong half to the filesystem and half not -- every filesystem on which devices and fifos can be created (which is not all of them) has two additional vnode ops tables for device and fifo vnodes respectively, which redirect most but not all of the ops to the code in miscfs/specfs and miscfs/fifofs. So things like chmod (VOP_SETATTR) go through the filesystem code and things like read and write go to the device or fifo. Buffers associated with a file are hung on that file's vnode. This is overloaded for devices such that buffers for the _device_ are hung on the device vnode... it works that way because the buffer cache is indexed by vnode and offset. (The buffer cache is both virtually and physically indexed: you can have buffers for an offset in a file, and buffers for a raw disk address, and the latter use the disk device's vnode as part of the key.) We've had assorted problems in the past caused by this overloading; if you remember when wapbl was rolled out there were assorted problems when wapbl was enabled on the root fs; this was caused by calling the root fs's operations for device buffers that came from some other filesystem, which then tried to do wapbl operations where no wapbl goop existed and croaked. We think those issues have pretty much all been found and fixed, but I'm wary of adding new ones. One of the goals of the device plumbing changes I proposed a couple months ago is to get rid of this overloading by making operating device vnodes separate from the on-disk special file vnodes. But that's a big invasive change and not happening in the near future. -- David A. Holland dholl...@netbsd.org
Re: VOP_STRATEGY for devices and pipes
On Thu, Jul 15, 2021 at 02:55:10AM +, David Holland wrote: > So I think what I would like to do is, for now at least (the device > plumbing changes I posted about a couple months back would eventually > change the situation) just prohibit extended attributes on device > nodes entirely. > > (I would not be opposed to also prohibiting them on fifos and > reverting this change for the time being, if only because it means I > wouldn't have to redo the patches I was hoping to commit this week...) > > Thoughts? Also, am I missing something? > > -- > David A. Holland > dholl...@netbsd.org setting an extattr on a device node in UFS is already disallowed. ffs_setextattr() has this check: if (ap->a_vp->v_type == VCHR || ap->a_vp->v_type == VBLK) return (EOPNOTSUPP); I have not checked other file systems to see if they have an equivalent check. I think we should continue to support extattrs on fifos for two reasons: - extattrs are used to implement ACLs, and it is useful to be able to set an ACL on a fifo. - it is good to remain more compatible with FreeBSD in this area. -Chuck
Re: VOP_STRATEGY for devices and pipes
> On Jul 14, 2021, at 9:14 PM, Paul Goyette wrote: > > Seems to me the best/safest thing to do for now would be to prohibit > the extended-attr on devices and fifos. That seems at best a sort-term workaround, but perhaps warranted to eliminate the data corruption issue. -- thorpej
Re: VOP_STRATEGY for devices and pipes
On Wed 14 Jul 2021 at 21:14:16 -0700, Paul Goyette wrote: > Seems to me the best/safest thing to do for now would be to prohibit > the extended-attr on devices and fifos. If I have a normal file system, and in a directory is an entry with an inode for a fifo (or a device), and I set an extended attribute on that inode, then surely it will get stored in the file system, and not in the fifo (or device)? So if the fifo (or device) code gets involved in this, then the bug is there, I'd think? -Olaf. -- ___ "Buying carbon credits is a bit like a serial killer paying someone else to \X/ have kids to make his activity cost neutral." -The BOFHfalu.nl@rhialto signature.asc Description: PGP signature
Re: VOP_STRATEGY for devices and pipes
Seems to me the best/safest thing to do for now would be to prohibit the extended-attr on devices and fifos. On Thu, 15 Jul 2021, David Holland wrote: I noticed that there was a loose function ffsext_strategy lying around, mentioned it to Christos, and he committed this: > Modified Files: >src/sys/ufs/ffs: ffs_vnops.c > > Log Message: > Hook up ffsext_strategy to fifos. Pointed out by dholland@ which is well and good (actually it isn't, it blew up a vnode tables cleanup patch I was preparing, but never mind that for now) but: If this is needed for fifos, isn't it also needed for devices? The ffs extended attribute blocks are stored in the buffer cache as negative offsets into the file they belong to (distinct, one hopes, from the negative offsets used for indirect blocks), which means that sooner or later they'll get written out, and when they do that happens via VOP_STRATEGY on the vnode they belong to. For fifos, without the above change, this will panic; VOP_STRATEGY on all fifos was/is genfs_badop. So probably trying to set extended attributes on a fifo will crash. For devices, though, the same thing will happen: buffers at negative offsets will be hung on the device, but if not intercepted by ffs (currently they aren't) they'll be handled by spec_strategy and treated as physical disk blocks. Which if we're lucky, will crash, and if not, possibly write to negative offsets in one disk partition which are positive offsets in the previous one, and corrupt the filesystem. So I think ffs also needs an ffsspec_strategy that does the same interception. Except, I'm not convinced that it's a good idea to try to mix the filesystem's metadata blocks with the device's own raw blocks like this. I don't know of anything that tries to use negative offsets on a device to mean something special, but I don't know there isn't anything either, in which case combining the two would make a horrible mess. But even absent that, it seems like having these entirely different blocks queueing in the same place is a bad idea begging for trouble. So I think what I would like to do is, for now at least (the device plumbing changes I posted about a couple months back would eventually change the situation) just prohibit extended attributes on device nodes entirely. (I would not be opposed to also prohibiting them on fifos and reverting this change for the time being, if only because it means I wouldn't have to redo the patches I was hoping to commit this week...) Thoughts? Also, am I missing something? -- David A. Holland dholl...@netbsd.org !DSPAM:60efa3bd146021739713255! ++--+--+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses:| | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com| | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org | || | pgoyett...@gmail.com | ++--+--+
