[OpenAFS-devel] Re: [PATCH v2] afs, bash: Fix open(O_CREAT) on an extant AFS file in a sticky dir

[Jeffrey E Altman]

I performed a review of the usage of vfsuid_eq_kuid() and vfsuid_eq().



On 5/13/2025 4:30 AM, David Howells wrote:

I mostly agree with David's conclusions and add some additional insight into the behavior of AFS servers.

Christian Brauner <brau...@kernel.org> wrote:

There's a few other places where we compare vfsuids:

* may_delete()
   -> check_sticky()
      -> __check_sticky()

* may_follow_link()

* may_linkat()

* fsuidgid_has_mapping()

Anyone of those need special treatment on AFS as well?

That's a good question.  I think it might be better to switch back to the v1
patch - which gives me two separate ops and provide a couple of vfs wrappers
for them and use them more widely.

So, perhaps:

        vfs_have_same_owner(inode1, inode2)

which indicates if the two inodes have the same ownership and:

        vfs_is_owned_by_me(inode)

which compares the inode's ownership to current_fsuid() by default.

The use of two distinct inode operations make the most sense to me.  An alternative is to provide one inode operation which sets two boolean output parameters: int (*check_ownership)(struct inode *const inode, struct inode *const parent,int *is_owned_by_me, int *is_owned_by_parent); where 'is_owned_by_me' or 'is_owned_by_parent' might be NULL if the answer is not required. However, I prefer David's suggestion.

The following places need to be considered for being changed:

  (*) chown_ok()
  (*) chgrp_ok()

      Should call vfs_is_owned_by_me().  Possibly these need to defer all their
      checks to the network filesystem as the interpretation of the target
      UID/GID depends on the netfs.

Since the late 1980s, afs servers do not permit changes to owner or group on files unless the caller is a member of the system:administrators group.  The file system clients cannot make this determination themselves.  If Linux wishes to further restrict the operation to current owner, then use of a vfs_is_owned_by_me() like inode operation should be used. Something to consider for future AFS3 or YFS protocol changes is to report the right to chown|chgrp to the client as part of a the FetchStatus result set.

  (*) do_coredump()

      Should probably call vfs_is_owned_by_me() to check that the file created
      is owned by the caller - but the check that's there might be sufficient.

I agree.

  (*) inode_owner_or_capable()

      Should call vfs_is_owned_by_me().

I agree.

      I'm not sure whether the namespace
      mapping makes sense in such a case, but it probably could be used.

  (*) vfs_setlease()

      Should call vfs_is_owned_by_me().  Actually, it should query if leasing
      is permitted.

      Also, setting locks could perhaps do with a permission call to the
      filesystem driver as AFS, for example, has a lock permission bit in the
      ACL, but since the AFS server checks that when the RPC call is made, it's
      probably unnecessary.

The AFS server will grant locks based upon the following rules:

 * the caller is granted the PRSFS_LOCK right (Shared lock only)
 * the caller is granted the PRSFS_WRITE right (Shared or Exclusive lock)
 * the caller is the file owner and is granted the PRSFS_INSERT right
   (Shared or Exclusive lock)

The client has enough information to implement a lock permission check if there was such an inode operation.

  (*) acl_permission_check()
  (*) posix_acl_permission()

      UIDs are part of these ACLs, so no change required.  AFS implements its
      own ACLs and evaluates them in ->permission() and on the server.

acl_permission_check() and posix_acl_permission() will not be called for AFS.  However, it it probably worth adding the vfs_is_owned_by_me() to acl_permission_check() in case there is another network filesystem which requires non-uid ownership checks and wants to use generic_permission().

  (*) may_follow_link()

      Should call vfs_is_owned_by_me() and also vfs_have_same_owner() on the
      the link and its parent dir.  The latter only applies on world-writable
      sticky dirs.

I agree

  (*) may_create_in_sticky()

      The initial subject of this patch.  Should call vfs_is_owned_by_me() and
      also vfs_have_same_owner() both.

I agree.

  (*) __check_sticky()

      Should call vfs_is_owned_by_me() on both the dir and the inode.

I agree.

  (*) may_dedupe_file()

      Should call vfs_is_owned_by_me().

I agree.

  (*) IMA policy ops.

      No idea.

I am not familiar with the Integrity Measurement Operations. However, looking at the usage of the ima_rule_entry fowner_op and fgroup_op operations, I do not believe the proposed vfs_is_owned_by_me() could be used to implement fowner_op.  If IMA should work filesystems which cannot rely upon local uid comparisons for owner and group, then I think the IMA fowner_op and fgroup_op would require an alternative implementation.  At the moment, IMA is unlikely to work properly with AFS.

David

Jeffrey Altman

smime.p7s
Description: S/MIME Cryptographic Signature