zfs_vnops_os.c: Do not allow a duplicate attribute of same name

[rmacklem]

Without this patch, if an attribute in user namespace is created while xattr=sa, it is possible to create an attribute of the same name via named attribute when xattr=dir.

This patch adds code to check for the duplicate in the sa block and deleted it when an attribute of the same name is created in the named attribute directory.

Fixes: #17540

Motivation and Context

Without this patch, if an attribute in user namespace is created while the xattr property is set to "sa", it is possible to create an attribute of the same name via named attributes after the xattr property is changed to "dir".

Description

The code in zfs_deleteextattr_sa() was factored out into zfs_deleteextattr_sa_impl(), so that it could be called without the vop_deleteextattr_args argument. Then zfs_deleteextattr_sa_impl() is used by a new function called zfs_delxattr_sa_dircreate() to delete any "sa" attribute of the same name. Then zfs_delxattr_sa_dircreate() is called from zfs_freebsd_create() when a named attribute is being created.

How Has This Been Tested?

On an up-to-date FreeBSD system with #17540 applied, an attribute was created via setextattr(1) when the file system's
xattr property was set to xattr=sa.

$ setextattr user <attribute-name> XXX <file>

Then, the xattr property was switched to "dir" and creation of a named attribute of the same name was done via:

$ runat <file> cp /etc/hosts <attribute-name>
$ lsextattr user <file>

Without this patch, <attribute-name> is listed twice, whereas it is only listed once if the above is done with this patch applied.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[amotin]

One possible issue I have noticed is that unlike other extattr operations doing similar things, here you are taking the z_xattr_lock inside zfs_delxattr_sa_dircreate() after zfs_create() succeeded. I can see that it is better than nothing, but it seems not atomic, and some other thread may still see two copies of the attribute for a short time. But I am not deep/fresh enough on this code to say if anything better can be done. I approve this if there is no other way, but take it with a grain of salt.

[rmacklem]

On Tue, Aug 5, 2025 at 7:36 AM Alexander Motin ***@***.***> wrote: CAUTION: This email originated from outside of the University of Guelph. Do not click links or open attachments unless you recognize the sender and know the content is safe. If in doubt, forward suspicious emails to ***@***.*** @amotin approved this pull request. One possible issue I have noticed is that unlike other extattr operations doing similar things, here you are taking the z_xattr_lock inside zfs_delxattr_sa_dircreate() after zfs_create() succeeded. I can see that it is better than nothing, but it seems not atomic, and some other thread may still see two copies of the attribute for a short time. But I am not deep/fresh enough on this code to say if anything better can be done. I approve this if there is no other way, but take it with a grain of salt.

Yes, I think you are correct. There are a couple of possible ways to fix this... - Move the acquisition of the lock to before the create. (It appears that is what happens for zfs_setextattr(), so it might be ok. There is a case where lookup of the directory is done with the lock held, so I doubt there is a LOR?) #2 - Just reverse the order of the zfs_listextattr_sa() and zfs_listextattr_dir() calls in zfs_listextattr_impl(). (The dir call should block until the named attr dir is unlocked and that will guarantee that it does not find the "sa" one.) I'd like to spend more time looking at this and doing some testing. Is it ok to leave this for a separate commit? rick

…

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: ***@***.***>

[amotin]

Is it ok to leave this for a separate commit?

If you have a good reason to hurry with this, then I don't have strong objections. Sure I would would prefer a proper patch from the beginning, but the world is not perfect.

[rmacklem]

Ok, I'll try and update the pull request in 2-3 days. Thanks for looking at it and spotting the problem, rick

…

On Tue, Aug 5, 2025 at 3:22 PM Alexander Motin ***@***.***> wrote: CAUTION: This email originated from outside of the University of Guelph. Do not click links or open attachments unless you recognize the sender and know the content is safe. If in doubt, forward suspicious emails to ***@***.*** amotin left a comment (openzfs/zfs#17593) Is it ok to leave this for a separate commit? If you have a good reason to hurry with this, then I don't have strong objections. Sure I would would prefer a proper patch from the beginning, but the world is not perfect. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: ***@***.***>

[rmacklem]

I am going to close this pull request. It was lucky you
mentioned the z_xattr_lock. It turns out that acquiring
it in zfs_freebsd_create() is not safe, because zfs_freebsd_create()
might be called with a locked named attribute directory.

In zfs_listextattr_dir(), it locks z_xattr_lock first and then the
named attribute dir (via namei() for ".").
--> This LOR could result in a deadlock.

I can't think of a way to handle this safely, so I think the
possibility of duplicate names (one in the "sa" and the
other in the "dir) will just have to be a "quirk" for now.
(So long as users set "xattr=dir" when they create the
file system, there won't be any duplicates.)

It is safe for the old extattr stuff because those VOPs
are always called with a regular vnode locked and never a
named attribute directory vnode locked.

I will keep looking for a "safe" way to avoid duplicates,
but I am not convinced it is doable?

[rmacklem]

On Tue, Aug 5, 2025 at 3:22 PM Alexander Motin ***@***.***> wrote: CAUTION: This email originated from outside of the University of Guelph. Do not click links or open attachments unless you recognize the sender and know the content is safe. If in doubt, forward suspicious emails to ***@***.*** *amotin* left a comment (openzfs/zfs#17593) <#17593 (comment)> Is it ok to leave this for a separate commit? If you have a good reason to hurry with this, then I don't have strong objections. Sure I would would prefer a proper patch from the beginning, but the world is not perfect.

As I think you've seen, I've closed the pull request. I now see that the z_xattr_lock would cause a LOR and possible deadlock. I haven't figured out any way to code around this LOR/deadlock since the vnode for the named attribute directory may already be locked when VOP_LOOKUP() and VOP_CREATE() is called. However, I also haven't figured out why z_xattr_lock exists at all. The vnode locking serializes calls to zfs_listextattr() and friends on a per-file basis. All that z_xattr_lock seems to do is make that serialization global (but not always, since it can be acquired read/shared). I'll need to look at this more. If z_xattr_lock is just cruft carried over from the Linux port and could be deleted, then my patch would be easy to fix. (It would need to acquire an exclusive lock on the file in zfs_freebsd_create() for the case where the argument is the named attribute directory. For now, I am documenting the behaviour in FreeBSD's named_attribute(7) man page. Thanks for spotting this, rick —

…

Reply to this email directly, view it on GitHub <#17593 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APNAL2QKSN3WJRZQKRQAW3T3MEVAVAVCNFSM6AAAAACDDQMJNKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCNJWHAYTIMJRGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>