On Wed, Apr 9, 2025 at 9:43 AM Shawn Webb <shawn.w...@hardenedbsd.org> wrote:
>
> On Sun, Apr 06, 2025 at 02:52:28PM -0700, Rick Macklem wrote:
> > On Sat, Apr 5, 2025 at 5:45 PM Shawn Webb <shawn.w...@hardenedbsd.org>
> > wrote:
> > >
> > > On Sat, Apr 05, 2025 at 05:36:07PM -0700, Rick Macklem wrote:
> > > > On Sat, Apr 5, 2025 at 4:43 PM Shawn Webb <shawn.w...@hardenedbsd.org>
> > > > wrote:
> > > > >
> > > > > On Sat, Apr 05, 2025 at 04:12:15PM -0700, Rick Macklem wrote:
> > > > > > On Sat, Apr 5, 2025 at 9:12 AM Shawn Webb
> > > > > > <shawn.w...@hardenedbsd.org> wrote:
> > > > > > >
> > > > > > > On Sat, Apr 05, 2025 at 08:52:06AM -0700, Rick Macklem wrote:
> > > > > > > > On Sat, Apr 5, 2025 at 8:50 AM Rick Macklem
> > > > > > > > <rick.mack...@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Apr 4, 2025 at 6:27 PM Shawn Webb
> > > > > > > > > <shawn.w...@hardenedbsd.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Sat, Apr 05, 2025 at 01:04:25AM +0000, Shawn Webb wrote:
> > > > > > > > > > > On Fri, Apr 04, 2025 at 05:40:21PM -0700, Rick Macklem
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Fri, Apr 4, 2025 at 10:50 AM Shawn Webb
> > > > > > > > > > > > <shawn.w...@hardenedbsd.org> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Thu, Apr 03, 2025 at 06:12:59PM -0700, Rick
> > > > > > > > > > > > > Macklem wrote:
> > > > > > > > > > > > > > On Thu, Apr 3, 2025 at 4:52 PM Shawn Webb
> > > > > > > > > > > > > > <shawn.w...@hardenedbsd.org> wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Wed, Apr 02, 2025 at 01:51:26PM -0700, Rick
> > > > > > > > > > > > > > > Macklem wrote:
> > > > > > > > > > > > > > > > The commit 2ec2ba7e232d just hit main. I do
> > > > > > > > > > > > > > > > not think it will
> > > > > > > > > > > > > > > > cause problems, but it is fairly large.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Man page updates will be done as separate
> > > > > > > > > > > > > > > > commits.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Hopefully this will not cause grief, rick
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Hey Rick,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > The patch review test plan mentions a patch to
> > > > > > > > > > > > > > > ZFS itself to support
> > > > > > > > > > > > > > > named attributes. Is that patch available
> > > > > > > > > > > > > > > somewhere?
> > > > > > > > > > > > > > The ZFS patch is currently in phabricator as D49654.
> > > > > > > > > > > > > > Feel free to review it.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > It can also be found at:
> > > > > > > > > > > > > > https://people.freebsd.org/~rmacklem/zfs-xattr.patch
> > > > > > > > > > > > > > (this is a smaller diff which can be applied to an
> > > > > > > > > > > > > > up-to-date main src
> > > > > > > > > > > > > > tree easily)
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hey Rick,
> > > > > > > > > > > > >
> > > > > > > > > > > > > I applied that zfs patch, but trying pathconf(2) on a
> > > > > > > > > > > > > file on a ZFS
> > > > > > > > > > > > > dataset with xattr=on (which seems to be the default)
> > > > > > > > > > > > > returns 0. Am I
> > > > > > > > > > > > > doing something wrong?
> > > > > > > > > > > > >
> > > > > > > > > > > > > ==== BEGIN LOG ====
> > > > > > > > > > > > > hbsd-current-01[shawn]:/home/shawn/tmp $ ./xattrtest
> > > > > > > > > > > > > xattrtest
> > > > > > > > > > > > > xattrtest: Named attributes not enabled: No error: 0
> > > > > > > > > > > > > hbsd-current-01[shawn]:/home/shawn/tmp (1) $ zfs list
> > > > > > > > > > > > > /usr/home/shawn
> > > > > > > > > > > > > NAME USED AVAIL REFER MOUNTPOINT
> > > > > > > > > > > > > rpool/usr/home 10.4G 71.4G 9.85G /usr/home
> > > > > > > > > > > > > hbsd-current-01[shawn]:/home/shawn/tmp $ zfs get
> > > > > > > > > > > > > xattr rpool/usr/home
> > > > > > > > > > > > > NAME PROPERTY VALUE SOURCE
> > > > > > > > > > > > > rpool/usr/home xattr on default
> > > > > > > > > > > > > ==== END LOG ====
> > > > > > > > > > > > >
> > > > > > > > > > > > > That xattrtest application is yours from:
> > > > > > > > > > > > > https://people.freebsd.org/~rmacklem/xattrtest.c
> > > > > > > > > > > > No idea. It works for me. You used up-to-date kernel
> > > > > > > > > > > > sources?
> > > > > > > > > > > > (Check that VIRF_NAMEDATTR is defined in
> > > > > > > > > > > > sys/sys/vnode.h.)
> > > > > > > > > > > > Oh, and one more thing to check. zfs_xattr_compat needs
> > > > > > > > > > > > to be non-zero.
> > > > > > > > > > > > (It's found in
> > > > > > > > > > > > sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c.
> > > > > > > > > > > > It's initialized to 1 and I don't see anything that
> > > > > > > > > > > > sets it to 0?)
> > > > > > > > > > >
> > > > > > > > > > > It is indeed set to 1.
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > The only thing I can think if is, if you changed xattr
> > > > > > > > > > > > to on, you need to
> > > > > > > > > > > > reboot (or at least remount) to get it to take effect.
> > > > > > > > > > > > (Maybe try setting it to "dir" and then reboot/remount.
> > > > > > > > > > > > Maybe there is
> > > > > > > > > > > > a difference between "on" and "dir"?)
> > > > > > > > > > >
> > > > > > > > > > > Yeah, tried rebooting and still no go. Note that xattr
> > > > > > > > > > > defaults to
> > > > > > > > > > > "on" in FreeBSD by default. My src tree is synced up to
> > > > > > > > > > > FreeBSD commit
> > > > > > > > > > > 7e70d94acd68b3ac6b45f49d4ab7a0f7867c3ea7. I brought in
> > > > > > > > > > > the ZFS patch
> > > > > > > > > > > you linked to.
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Or, did you build zfs.ko some other way than as part of
> > > > > > > > > > > > a kernel build?
> > > > > > > > > > > > (It needs the patched .h files in the kernel sources,
> > > > > > > > > > > > not something in
> > > > > > > > > > > > /usr/include/sys
> > > > > > > > > > > > that has not yet been updated.)
> > > > > > > > > > > > All the ZFS changes are #ifdef'd, since OpenZFS
> > > > > > > > > > > > requires the sources
> > > > > > > > > > > > build for older kernels. (Basically #ifdef'd on that
> > > > > > > > > > > > VIRF_NAMEDATTR mentioned
> > > > > > > > > > > > above.)
> > > > > > > > > > >
> > > > > > > > > > > Perhaps I need to do a clean build. I'll try that and
> > > > > > > > > > > report back.
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > It does remind me that I need to try a build of zfs.ko
> > > > > > > > > > > > by doing a "make" in
> > > > > > > > > > > > the module directory.
> > > > > > > > > > > >
> > > > > > > > > > > > You can try the attached trivial patch and see if it
> > > > > > > > > > > > spits out "pathconf ret=1"
> > > > > > > > > > > > on the console.
> > > > > > > > > > >
> > > > > > > > > > > I'll try that after a clean rebuild of the kernel.
> > > > > > > > > >
> > > > > > > > > > Clean rebuild didn't resolve it. However, I made some
> > > > > > > > > > progress.
> > > > > > > > > >
> > > > > > > > > > I created a dataset specifically for this since I can't
> > > > > > > > > > really unmount
> > > > > > > > > > my /usr/home dataset, so my example down below is a bit
> > > > > > > > > > different than
> > > > > > > > > > the previous example I gave.
> > > > > > > > > >
> > > > > > > > > > The xattr property is set to "on" for the dataset. However,
> > > > > > > > > > it's not
> > > > > > > > > > mounted with the xattr property. In order to get it to
> > > > > > > > > > work, I had to
> > > > > > > > > > run the following commands:
> > > > > > > > > >
> > > > > > > > > > ==== BEGIN LOG ====
> > > > > > > > > > $ sudo zfs umount rpool/scratch/xattr
> > > > > > > > > > $ sudo mount -t zfs -o xattr rpool/scratch/xattr
> > > > > > > > > > /scratch/xattr
> > > > > > > > > > $ zfs get xattr rpool/scratch/xattr
> > > > > > > > > > NAME PROPERTY VALUE SOURCE
> > > > > > > > > > rpool/scratch/xattr xattr on local
> > > > > > > > > > $ mount | grep xattr
> > > > > > > > > > rpool/scratch/xattr on /scratch/xattr (zfs, local, noatime,
> > > > > > > > > > nfsv4acls, named attributes)
> > > > > > > > > > $ mount | grep named
> > > > > > > > > > rpool/scratch/xattr on /scratch/xattr (zfs, local, noatime,
> > > > > > > > > > nfsv4acls, named attributes)
> > > > > > > > > > ==== END LOG ====
> > > > > > > > > >
> > > > > > > > > > So it looks like FreeBSD does not honor the xattr zfs
> > > > > > > > > > property,
> > > > > > > > > > otherwise you would see a whole bunch of datasets mounted
> > > > > > > > > > with the
> > > > > > > > > > "named attributes" flag in that last `mount | grep` command.
> > > > > > > > > I've looked at this a little more...
> > > > > > > > > There is a function xattr_changed_cb() that updates the xattr
> > > > > > > > > property
> > > > > > > > > information.
> > > > > > > > > It gets called when "zfs set xattr=XX <file-system>" is done
> > > > > > > > > or when
> > > > > > > > > the mount option "xattr" is set.
> > > > > > > > >
> > > > > > > > > The question seems to be "Why was the stuff not correctly set
> > > > > > > > > for your file
> > > > > > > > > systems, although they show xattr=on?"
> > > > > > >
> > > > > > > This is indeed what I was inferring. xattr has been set to "on"
> > > > > > > since
> > > > > > > the pool's creation as far as I can tell. Until experimenting with
> > > > > > > your patch, I didn't really even know the xattr property even
> > > > > > > existed.
> > > > > > I was able to reproduce this with a fresh zpool.
> > > > > > Although it reported "xattr on", that is not really accurate.
> > > > > >
> > > > > > I stuck a printf() in here (line#853-861 of
> > > > > > sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vfsops.)
> > > > > >
> > > > > > /* should either have both of these objects or none */
> > > > > > error = zap_lookup(os, MASTER_NODE_OBJ, ZFS_SA_ATTRS, 8, 1,
> > > > > > &sa_obj);
> > > > > > if (error != 0)
> > > > > > return (error);
> > > > > >
> > > > > > error = zfs_get_zplprop(os, ZFS_PROP_XATTR, &val);
> > > > > > --> The printf here shows that error == ENOENT.
> > > > > > if (error == 0 && val == ZFS_XATTR_SA)
> > > > > > zfsvfs->z_xattr_sa = B_TRUE;
> > > > > >
> > > > > > The printf() of error showed ENOENT. So, "xattr" actually does not
> > > > > > exist. "zfs get xattr <file-system>" calls it "on" but that's not
> > > > > > accurate.
> > > > > >
> > > > > > As root, I did:
> > > > > > # zfs set xattr=dir <file-system>
> > > > > > - reboot
> > > > > >
> > > > > > This makes it work, although the above call still returns ENOENT.
> > > > > > It looks to me like zfsvfs_init() gets called from zfsvfs_create()
> > > > > > right at the beginning of zfs_domount().
> > > > > > Later in zfs_domount() it calls
> > > > > > zfsvfs_setup()->zfs_register_callbacks(),
> > > > > > which now finds the property (because of the "zfs set xattr=dir
> > > > > > <file-system>")
> > > > > > and registers it via dsl_prop_register().
> > > > > >
> > > > > > I don't know if the above is correct?
> > > > > > Personally, I would prefer to see "zfs get xattr <file-system>"
> > > > > > reply "not set"
> > > > > > instead of "on".
> > > > > >
> > > > > > rick
> > > > > >
> > > > > >
> > > > > > Setting the property makes it work, but only after rebooting
> > > > > > (a umount/mount would probably have the same effect, I think?).
> > > > >
> > > > > So I wonder why neither an unmount/mount nor a reboot causes `mount |
> > > > > grep "named attribute"` to fail for me. HardenedBSD doesn't have any
> > > > > changes to ZFS that would cause that kind of behavior. I have to
> > > > > unmount, but manually mount with `mount -t zfs -o xattr ...` in order
> > > > > to access the new feature you wrote. Weird.
> > > > So, you have done
> > > > # zfs set xattr=dir <file-system>
> > > > followed by a reboot and it still doesn't work?
> > >
> > > Correct, even a reboot does not work.
> > No idea. If you apply the atteched little patch (just printf's) and post
> > what gets printed out (when a mount that doesn't work is done),
> > it might help explain it.
>
> Turns out this was a big case of PEBCAK. I had misunderstood which
> value the xattr property should have been set to. After setting it to
> "dir" and rebooting, the ZFS datasets are now mounted with named
> attributes support.
Sounds good. I'll admit it would be nice if "zfs get xattr" reported
"dir" instead
of "on", but that's for another day.
>
> Thanks for your patience and I apologize for the noise. Now it's time
> to figure out how best to prevent fdlopen(extattrfd). :-)
At this time (I haven't yet been able to verify that Solaris does it this way),
permission to access the named attribute directory is defined by the permissions
on the parent object.
However, the individual attributes are regular files and permission is
controlled
for them the same way as any other regular file (mode, uid, gid, ACL).
(So, I don't think there is any difference for fdlopen() from any
other regular file's
fd argument?)
Have fun with it, rick
ps: The access permission stuff is in the ZFS patch, so can be changed if that
seems appropriate.
>
> --
> Shawn Webb
> Cofounder / Security Engineer
> HardenedBSD
>
> Signal Username: shawn_webb.74
> Tor-ified Signal: +1 303-901-1600 / shawn_webb_opsec.50
> https://git.hardenedbsd.org/hardenedbsd/pubkeys/-/raw/master/Shawn_Webb/03A4CBEBB82EA5A67D9F3853FF2E67A277F8E1FA.pub.asc
