Message-ID: <B0E4F345-9958-44C2-9985-96F77F0DEF0F@fb.com>
Date: Fri, 26 Jul 2024 07:01:39 +0000
From: Song Liu <songliubraving@...a.com>
To: Al Viro <viro@...iv.linux.org.uk>
CC: Song Liu <song@...nel.org>, bpf <bpf@...r.kernel.org>,
        Linux-Fsdevel
	<linux-fsdevel@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Kernel
 Team <kernel-team@...a.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Eduard
 Zingerman <eddyz87@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel
 Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        "brauner@...nel.org" <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
        KP Singh
	<kpsingh@...nel.org>,
        "mattbobrowski@...gle.com" <mattbobrowski@...gle.com>
Subject: Re: [PATCH bpf-next 1/2] bpf: Add kfunc bpf_get_dentry_xattr() to
 read xattr from dentry

Hi Al, 

Thanks for your quick reply. 

> On Jul 25, 2024, at 10:34 PM, Al Viro <viro@...iv.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.

Powered by blists - more mailing lists