The primary use case here is to read xattr for directories from BPF
programs. This feature enables tagging all the files in a directory
with a xattr on the directory. More specifically, starting from LSM hook
security_file_open(), we can read xattr of the file's parent directories.

To have referenced access to dentry, a few more kfuncs are added:
  - bpf_file_dentry
  - bpf_dget_parent
  - bpf_dput

Both bpf_file_dentry and bpf_dget_parent take a reference to the dentry
(KF_ACQUIRE), which has to be released by bpf_dput (KF_RELEASE). This
makes sure only trusted pointers (KF_TRUSTED_ARGS) can be used for the
dentry.

Note that, file_dentry() doesn't take reference to the dentry, but
bpf_file_dentry() does.

Signed-off-by: Song Liu <song@kernel.org>
---
 kernel/trace/bpf_trace.c | 60 ++++++++++++++++++++++++++++++++++------
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index cd098846e251..b8e6eeabb773 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1443,22 +1443,21 @@ late_initcall(bpf_key_sig_kfuncs_init);
 __bpf_kfunc_start_defs();
 
 /**
- * bpf_get_file_xattr - get xattr of a file
- * @file: file to get xattr from
+ * bpf_get_dentry_xattr - get xattr of a dentry
+ * @dentry: dentry to get xattr from
  * @name__str: name of the xattr
  * @value_p: output buffer of the xattr value
  *
- * Get xattr *name__str* of *file* and store the output in *value_ptr*.
+ * Get xattr *name__str* of *dentry* and store the output in *value_ptr*.
  *
  * For security reasons, only *name__str* with prefix "user." is allowed.
  *
  * Return: 0 on success, a negative value on error.
  */
-__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
-				   struct bpf_dynptr *value_p)
+__bpf_kfunc int bpf_get_dentry_xattr(struct dentry *dentry, const char *name__str,
+				     struct bpf_dynptr *value_p)
 {
 	struct bpf_dynptr_kern *value_ptr = (struct bpf_dynptr_kern *)value_p;
-	struct dentry *dentry;
 	u32 value_len;
 	void *value;
 	int ret;
@@ -1471,20 +1470,63 @@ __bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
 	if (!value)
 		return -EINVAL;
 
-	dentry = file_dentry(file);
 	ret = inode_permission(&nop_mnt_idmap, dentry->d_inode, MAY_READ);
 	if (ret)
 		return ret;
 	return __vfs_getxattr(dentry, dentry->d_inode, name__str, value, value_len);
 }
 
+/**
+ * bpf_get_file_xattr - get xattr of a file
+ * @file: file to get xattr from
+ * @name__str: name of the xattr
+ * @value_p: output buffer of the xattr value
+ *
+ * Get xattr *name__str* of *file* and store the output in *value_ptr*.
+ *
+ * For security reasons, only *name__str* with prefix "user." is allowed.
+ *
+ * Return: 0 on success, a negative value on error.
+ */
+__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
+				   struct bpf_dynptr *value_p)
+{
+	struct dentry *dentry;
+
+	dentry = file_dentry(file);
+	return bpf_get_dentry_xattr(dentry, name__str, value_p);
+}
+
+__bpf_kfunc struct dentry *bpf_file_dentry(const struct file *file)
+{
+	/* file_dentry() does not hold reference to the dentry. We add a
+	 * dget() here so that we can add KF_ACQUIRE flag to
+	 * bpf_file_dentry().
+	 */
+	return dget(file_dentry(file));
+}
+
+__bpf_kfunc struct dentry *bpf_dget_parent(struct dentry *dentry)
+{
+	return dget_parent(dentry);
+}
+
+__bpf_kfunc void bpf_dput(struct dentry *dentry)
+{
+	return dput(dentry);
+}
+
 __bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(fs_kfunc_set_ids)
+BTF_ID_FLAGS(func, bpf_get_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_file_dentry, KF_TRUSTED_ARGS | KF_ACQUIRE)
+BTF_ID_FLAGS(func, bpf_dget_parent, KF_TRUSTED_ARGS | KF_ACQUIRE)
+BTF_ID_FLAGS(func, bpf_dput, KF_RELEASE)
 BTF_KFUNCS_END(fs_kfunc_set_ids)
 
-static int bpf_get_file_xattr_filter(const struct bpf_prog *prog, u32 kfunc_id)
+static int fs_kfunc_filter(const struct bpf_prog *prog, u32 kfunc_id)
 {
 	if (!btf_id_set8_contains(&fs_kfunc_set_ids, kfunc_id))
 		return 0;
@@ -1496,7 +1538,7 @@ static int bpf_get_file_xattr_filter(const struct bpf_prog *prog, u32 kfunc_id)
 static const struct btf_kfunc_id_set bpf_fs_kfunc_set = {
 	.owner = THIS_MODULE,
 	.set = &fs_kfunc_set_ids,
-	.filter = bpf_get_file_xattr_filter,
+	.filter = fs_kfunc_filter,
 };
 
 static int __init bpf_fs_kfuncs_init(void)
-- 
2.43.0

On Thu, Jul 25, 2024 at 04:47:05PM -0700, Song Liu wrote:

> +__bpf_kfunc struct dentry *bpf_file_dentry(const struct file *file)
> +{
> +	/* file_dentry() does not hold reference to the dentry. We add a
> +	 * dget() here so that we can add KF_ACQUIRE flag to
> +	 * bpf_file_dentry().
> +	 */
> +	return dget(file_dentry(file));
> +}
> +
> +__bpf_kfunc struct dentry *bpf_dget_parent(struct dentry *dentry)
> +{
> +	return dget_parent(dentry);
> +}
> +
> +__bpf_kfunc void bpf_dput(struct dentry *dentry)
> +{
> +	return dput(dentry);
> +}

	If you keep a file reference, why bother grabbing dentry one?
If not, you have a very bad trouble if that opened file is the only
thing that keeps the filesystem busy.

	It's almost certainly a wrong interface; please, explain what
exactly are you trying to do here.

Hi Al, 

Thanks for your quick reply. 

> On Jul 25, 2024, at 10:34 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> 
> On Thu, Jul 25, 2024 at 04:47:05PM -0700, Song Liu wrote:
> 
>> +__bpf_kfunc struct dentry *bpf_file_dentry(const struct file *file)
>> +{
>> + /* file_dentry() does not hold reference to the dentry. We add a
>> + * dget() here so that we can add KF_ACQUIRE flag to
>> + * bpf_file_dentry().
>> + */
>> + return dget(file_dentry(file));
>> +}
>> +
>> +__bpf_kfunc struct dentry *bpf_dget_parent(struct dentry *dentry)
>> +{
>> + return dget_parent(dentry);
>> +}
>> +
>> +__bpf_kfunc void bpf_dput(struct dentry *dentry)
>> +{
>> + return dput(dentry);
>> +}
> 
> If you keep a file reference, why bother grabbing dentry one?
> If not, you have a very bad trouble if that opened file is the only
> thing that keeps the filesystem busy.

Yes, we keep a file reference for the duration of the BPF program. 
Therefore, it is technically not necessary to grab a dentry one.
However, we grab a dentry reference to make the dentry pointer 
returned by bpf_file_dentry() a trusted pointer from BPF verifier's 
POV, so that these kfuncs are more robust. 

The following explanation is a bit long. Please let me know if it 
turns out confusing.


==== What is trusted pointer? ====

Trusted point is the mechanism to make sure bpf kfuncs are 
called with valid pointer. The BPF verifier requires certain BPF 
kfuncs (helpers) are called with trusted pointers. A pointer is 
trusted if one of the following two is true:

1. The pointer is passed directly by the tracepoint/kprobe, i.e., 
   no pointer walking, no non-zero offset. For example, 

   int bpf_security_file_open(struct file *file)  /* file is trusted */
   {
       /* mapping is not trusted */
       struct address_space    *mapping = file->f_mapping;

       /* file2 is not trusted */
       struct file *file2 = file + 1;
   }

2. The pointer is returned by a kfunc with KF_ACQUIRE. This pointer 
   has to be released by a kfunc with KF_RELEASE. KF_ACQUIRE and 
   KF_RELEASE kfuncs are like any _get() _put() pairs. 


==== bpf_dget_parent and bpf_dput ====

In this case, bpf_dget_parent() is a KF_ACQUIRE kfunc and 
bpf_dput() is a KF_RELEASE function. They are just like regular
_get() _put() functions. 

The BPF verifier makes sure pointers acquired by bpf_dget_parent() 
is always released by bpf_dput() before the BPF program returns. 
For example, in the following BPF program:

xxxx(struct dentry *d)
{
    struct dentry *parent = bpf_dget_parent(d); 

    /* main logic */
     
    bpf_dput(parent);
}

If the bpf_dput() call is missing, the verifier will not allow
the program to load. 


==== More on kfunc safety ====

Trusted point makes kfunc calls safe. In this case, we want 
bpf_get_dentry_xattr() to only take trusted dentry pointer. 
For example, in the security_inode_listxattr LSM hook:

bpf_security_inode_listxattr(struct dentry *dentry)
{
       /* This is allowed, dentry is an input and thus
        * is trusted 
        */
       bpf_get_dentry_xattr(dentry); 


       /* This is not allowed, as dentry->d_parent is 
        * not trusted
        */
       bpf_get_dentry_xattr(dentry->d_parent);


       /* This is allowed, as bpf_dget_parent() holds  
        * a reference to d_parent, and returns a trusted
        * pointer
        */
       struct dentry *parent = bpf_dget_parent(dentry);


       /* The following is needed, as we need the release 
        * parent pointer. If this line is missing, this
        * program cannot pass BPF verifier. 
        */
       bpf_dput(parent);
}


==== bpf_file_dentry ====

In this use case, we want to get from file pointer, such as
LSM hook security_file_open() to the dentry and thus walk the
directory tree. However, security_file_open() does not pass
in a dentry pointer, and file->f_path.dentry is not a trusted
pointer. There are two ways to get a trusted dentry pointer
from a file pointer:

1. As what we do here, use bpf_file_dentry() to hold a 
   reference on file->f_path.dentry and return a trusted 
   pointer. 
2. Give the verifier special knowledge that if file pointer
   is trusted, file->f_path.dentry is also trusted. This 
   can be achieve with the following macros:
      BTF_TYPE_SAFE_TRUSTED
      BTF_TYPE_SAFE_RCU
      BTF_TYPE_SAFE_RCU_OR_NULL. 

Using the second method here requires a little more work in the
BPF verifier, as dentry is not a simple pointer in struct file, 
but f_path.dentry. Therefore, I chose current approach that
bpf_file_dentry() holds a reference on dentry pointer, and the 
pointer has to be released with bpf_dput(). 

For more details about trusted pointers in kfuncs, please refer to 
Documentation/bpf/kfuncs.rst. 

Does this answer your question? 

Thanks,
Song


> It's almost certainly a wrong interface; please, explain what
> exactly are you trying to do here.