Re: BPF/Question: PTR_TRUSTED vs PTR_UNTRUSTED

Re: BPF/Question: PTR_TRUSTED vs PTR_UNTRUSTED

[David Vernet]

On Wed, Jun 14, 2023 at 08:06:18AM +0000, Matt Bobrowski wrote:
> Hey David,
> 
> I have a quick question in regards to the type tag PTR_TRUSTED, which
> I believe is causing one of my BPF programs to no longer successfully
> pass the BPF verifier. Given the following BPF program as an example:

Hi Matt,

Thanks for the message. I'll add the bpf list to cc so the community can
see and discuss.

> ---
> SEC("lsm.s/bprm_committed_creds")
> int BPF_PROG(bprm_committed_creds, struct linux_binprm *bprm)
> {
> 	int ret;
> 	char buf[64] = {0};
> 
> 	ret = bpf_d_path(&bprm->file->f_path, buf, sizeof(buf));
> 	if (ret < 0) {
> 		bpf_printk("bpf_d_path: ret=%d", ret);
> 		return 0;
> 	}
> 	
> 	return 0;
> }
> ---
> 
> In this case, the PTR_TO_BTF_ID pointer (&bprm->file->f_path) I
> imagine is considered to be trusted and can be passed to the BPF
> helper bpf_d_path without the BPF verifier complaining.
> 
> On the other hand, given the following relatively similar BPF program
> as an example:
> 
> ---
> SEC("lsm.s/bprm_committed_creds")
> int BPF_PROG(bprm_committed_creds)
> {
> 	int ret;
> 	char buf[64] = {0};
> 	struct task_struct *current = bpf_get_current_task_btf();
> 
> 	ret = bpf_d_path(&current->fs->pwd, buf, sizeof(buf));
> 	if (ret < 0) {
> 		bpf_printk("bpf_d_path: ret=%d", ret);
> 		return 0;
> 	}
> 	
> 	return 0;
> }
> ---
> 
> In this case, the PTR_TO_BTF_ID pointer (&current->fs->pwd) is
> considered to be untrusted and cannot be passed to the BPF helper
> bpf_d_path as the BPF verifier fails to load the BPF program with the
> following message:

The reason this is happening is that the struct fs_struct *fs field of
struct task_struct is not marked as inheriting its parent task's trusted
status. The following diff would fix the issue:

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index fa43dc8e85b9..8b8ccde342f9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5857,6 +5857,7 @@ BTF_TYPE_SAFE_RCU(struct task_struct) {
        struct css_set __rcu *cgroups;
        struct task_struct __rcu *real_parent;
        struct task_struct *group_leader;
+ struct fs_struct *fs;
 };

 BTF_TYPE_SAFE_RCU(struct cgroup) {
diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_failure.c b/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
index dcdea3127086..aef2d4689826 100644
--- a/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
+++ b/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
@@ -324,3 +324,24 @@ int BPF_PROG(task_kfunc_release_in_map, struct task_struct *task, u64 clone_flag

        return 0;
 }
+
+SEC("lsm.s/bprm_committed_creds")
+__success
+int BPF_PROG(bprm_committed_creds)
+{
+ int ret;
+ char buf[64] = {0};
+ struct task_struct *current = bpf_get_current_task_btf();
+ struct fs_struct *fs;
+
+ bpf_rcu_read_lock();
+ fs = current->fs;
+ ret = bpf_d_path(&fs->pwd, buf, sizeof(buf));
+ bpf_rcu_read_unlock();
+ if (ret < 0) {
+         bpf_printk("bpf_d_path: ret=%d", ret);
+         return 0;
+ }
+
+ return 0;
+}

With this patch, the above test would pass (meaning the program is successfully
verified).

> 
> ---
> ; ret = bpf_d_path(&current->fs->pwd, buf, sizeof(buf));
> 15: (b7) r3 = 64                      ; R3_w=64
> 16: (85) call bpf_d_path#147
> R1 type=untrusted_ptr_ expected=ptr_, trusted_ptr_, rcu_ptr_
> ---
> 
> What's interesting is that based on the following commit
> (3f00c52393445 bpf: Allow trusted pointers to be passed to
> KF_TRUSTED_ARGS kfuncs) message:
> 
> ---
> PTR_TRUSTED pointers are passed directly from the kernel as a
> tracepoint or struct_ops callback argument. Any nested pointer that is
> obtained from walking a PTR_TRUSTED pointer is no longer
> PTR_TRUSTED. From the example above, the struct task_struct *task
> argument is PTR_TRUSTED, but the 'nested' pointer obtained from
> 'task->last_wakee' is not PTR_TRUSTED.
> ---
> 
> I'm reading this as though the first example program should also fail
> BPF verification given that a nested pointer to struct path is
> obtained from walking a PTR_TRUSTED pointer, which presumably is
> struct linux_binprm in this case. What subtle details am I missing
> here? Why is that the first program loads, but the second does not?

The subtle details are in that these are different types. We can't assume that
a child pointer automatically inherits its parent's trusted status for all
types, so we have to hard code it for now until gcc supports using btf type
tags so this can be expressed with annotations like __trusted or __rcu.
Consider that some types may have NULL child pointers in certain scenarios.
Others may be valid as long as you access it in an RCU read region, others may
be valid as long as you access it in an RCU read region and it wasn't NULL.

The struct linux_binprm type's safety is specified in
kernel/bpf/verifier.c:

BTF_TYPE_SAFE_TRUSTED(struct linux_binprm) {
	struct file *file;
};

struct task_struct is specified above:

/* RCU trusted: these fields are trusted in RCU CS and never NULL */
BTF_TYPE_SAFE_RCU(struct task_struct) {
        const cpumask_t *cpus_ptr;
        struct css_set __rcu *cgroups;
        struct task_struct __rcu *real_parent;
        struct task_struct *group_leader;
};

> What workaround needs to be implemented in order to have the second
> example program satisfy the PTR_TRUSTED contract with the BPF helper
> bpf_d_path?

See above, we need to add struct fs_struct * to the list of trusted fields for
struct task_struct, and enclose it in an RCU read region.

Thanks,
David

Re: BPF/Question: PTR_TRUSTED vs PTR_UNTRUSTED

[Matt Bobrowski]

Hey David!

Thanks for getting back to me, and super sorry about the delay.

On Thu, Jun 15, 2023 at 12:40:33PM -0500, David Vernet wrote:
> On Wed, Jun 14, 2023 at 08:06:18AM +0000, Matt Bobrowski wrote:
> > Hey David,
> > 
> > I have a quick question in regards to the type tag PTR_TRUSTED, which
> > I believe is causing one of my BPF programs to no longer successfully
> > pass the BPF verifier. Given the following BPF program as an example:
> 
> Hi Matt,
> 
> Thanks for the message. I'll add the bpf list to cc so the community can
> see and discuss.
> 
> > ---
> > SEC("lsm.s/bprm_committed_creds")
> > int BPF_PROG(bprm_committed_creds, struct linux_binprm *bprm)
> > {
> > 	int ret;
> > 	char buf[64] = {0};
> > 
> > 	ret = bpf_d_path(&bprm->file->f_path, buf, sizeof(buf));
> > 	if (ret < 0) {
> > 		bpf_printk("bpf_d_path: ret=%d", ret);
> > 		return 0;
> > 	}
> > 	
> > 	return 0;
> > }
> > ---
> > 
> > In this case, the PTR_TO_BTF_ID pointer (&bprm->file->f_path) I
> > imagine is considered to be trusted and can be passed to the BPF
> > helper bpf_d_path without the BPF verifier complaining.
> > 
> > On the other hand, given the following relatively similar BPF program
> > as an example:
> > 
> > ---
> > SEC("lsm.s/bprm_committed_creds")
> > int BPF_PROG(bprm_committed_creds)
> > {
> > 	int ret;
> > 	char buf[64] = {0};
> > 	struct task_struct *current = bpf_get_current_task_btf();
> > 
> > 	ret = bpf_d_path(&current->fs->pwd, buf, sizeof(buf));
> > 	if (ret < 0) {
> > 		bpf_printk("bpf_d_path: ret=%d", ret);
> > 		return 0;
> > 	}
> > 	
> > 	return 0;
> > }
> > ---
> > 
> > In this case, the PTR_TO_BTF_ID pointer (&current->fs->pwd) is
> > considered to be untrusted and cannot be passed to the BPF helper
> > bpf_d_path as the BPF verifier fails to load the BPF program with the
> > following message:
> 
> The reason this is happening is that the struct fs_struct *fs field of
> struct task_struct is not marked as inheriting its parent task's trusted
> status. The following diff would fix the issue:
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fa43dc8e85b9..8b8ccde342f9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5857,6 +5857,7 @@ BTF_TYPE_SAFE_RCU(struct task_struct) {
>         struct css_set __rcu *cgroups;
>         struct task_struct __rcu *real_parent;
>         struct task_struct *group_leader;
> + struct fs_struct *fs;
>  };

Oh, right. So, if we explicitly dereference the struct fs_struct
member of struct task_struct within a RCU read-side critical section,
the BPF verifier considers the pointer to struct fs_struct as being
safe and trusted. Is that right?

Why is it that we need to explicitly add it to such lists so that
they're considered to be trusted and cannot simply perform the
bpf_rcu_read_lock/unlock() dance from within the BPF program? Also,
should we not add the field to BTF_TYPE_SAFE_RCU_OR_NULL() instead of
BTF_TYPE_SAFE_RCU(), as struct fs_struct could perhaps be NULL in some
circumstances?

Are you OK with me carrying this recommended patch to the mailing
list?

>  BTF_TYPE_SAFE_RCU(struct cgroup) {
> diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_failure.c b/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
> index dcdea3127086..aef2d4689826 100644
> --- a/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
> +++ b/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
> @@ -324,3 +324,24 @@ int BPF_PROG(task_kfunc_release_in_map, struct task_struct *task, u64 clone_flag
> 
>         return 0;
>  }
> +
> +SEC("lsm.s/bprm_committed_creds")
> +__success
> +int BPF_PROG(bprm_committed_creds)
> +{
> + int ret;
> + char buf[64] = {0};
> + struct task_struct *current = bpf_get_current_task_btf();
> + struct fs_struct *fs;
> +
> + bpf_rcu_read_lock();
> + fs = current->fs;
> + ret = bpf_d_path(&fs->pwd, buf, sizeof(buf));
> + bpf_rcu_read_unlock();
> + if (ret < 0) {
> +         bpf_printk("bpf_d_path: ret=%d", ret);
> +         return 0;
> + }
> +
> + return 0;
> +}
> 
> With this patch, the above test would pass (meaning the program is successfully
> verified).
> 
> > 
> > ---
> > ; ret = bpf_d_path(&current->fs->pwd, buf, sizeof(buf));
> > 15: (b7) r3 = 64                      ; R3_w=64
> > 16: (85) call bpf_d_path#147
> > R1 type=untrusted_ptr_ expected=ptr_, trusted_ptr_, rcu_ptr_
> > ---
> > 
> > What's interesting is that based on the following commit
> > (3f00c52393445 bpf: Allow trusted pointers to be passed to
> > KF_TRUSTED_ARGS kfuncs) message:
> > 
> > ---
> > PTR_TRUSTED pointers are passed directly from the kernel as a
> > tracepoint or struct_ops callback argument. Any nested pointer that is
> > obtained from walking a PTR_TRUSTED pointer is no longer
> > PTR_TRUSTED. From the example above, the struct task_struct *task
> > argument is PTR_TRUSTED, but the 'nested' pointer obtained from
> > 'task->last_wakee' is not PTR_TRUSTED.
> > ---
> > 
> > I'm reading this as though the first example program should also fail
> > BPF verification given that a nested pointer to struct path is
> > obtained from walking a PTR_TRUSTED pointer, which presumably is
> > struct linux_binprm in this case. What subtle details am I missing
> > here? Why is that the first program loads, but the second does not?
> 
> The subtle details are in that these are different types. We can't assume that
> a child pointer automatically inherits its parent's trusted status for all
> types, so we have to hard code it for now until gcc supports using btf type
> tags so this can be expressed with annotations like __trusted or __rcu.

Oh, interesting. You mention that this is explicitly needed for gcc,
so I'm now wondering how the semantics differ when using clang? Where
would such annotations (__trusted, __rcu) be applied?

> Consider that some types may have NULL child pointers in certain scenarios.
> Others may be valid as long as you access it in an RCU read region, others may
> be valid as long as you access it in an RCU read region and it wasn't NULL.

This makes sense. Thanks for the explanation.

> The struct linux_binprm type's safety is specified in
> kernel/bpf/verifier.c:
> 
> BTF_TYPE_SAFE_TRUSTED(struct linux_binprm) {
> 	struct file *file;
> };
> 
> struct task_struct is specified above:
> 
> /* RCU trusted: these fields are trusted in RCU CS and never NULL */
> BTF_TYPE_SAFE_RCU(struct task_struct) {
>         const cpumask_t *cpus_ptr;
>         struct css_set __rcu *cgroups;
>         struct task_struct __rcu *real_parent;
>         struct task_struct *group_leader;
> };
> 
> > What workaround needs to be implemented in order to have the second
> > example program satisfy the PTR_TRUSTED contract with the BPF helper
> > bpf_d_path?
> 
> See above, we need to add struct fs_struct * to the list of trusted fields for
> struct task_struct, and enclose it in an RCU read region.

/M

Re: BPF/Question: PTR_TRUSTED vs PTR_UNTRUSTED

[David Vernet]

On Thu, Jul 20, 2023 at 02:29:11PM +0000, Matt Bobrowski wrote:
> Hey David!
> 
> Thanks for getting back to me, and super sorry about the delay.
> 
> On Thu, Jun 15, 2023 at 12:40:33PM -0500, David Vernet wrote:
> > On Wed, Jun 14, 2023 at 08:06:18AM +0000, Matt Bobrowski wrote:
> > > Hey David,
> > > 
> > > I have a quick question in regards to the type tag PTR_TRUSTED, which
> > > I believe is causing one of my BPF programs to no longer successfully
> > > pass the BPF verifier. Given the following BPF program as an example:
> > 
> > Hi Matt,
> > 
> > Thanks for the message. I'll add the bpf list to cc so the community can
> > see and discuss.
> > 
> > > ---
> > > SEC("lsm.s/bprm_committed_creds")
> > > int BPF_PROG(bprm_committed_creds, struct linux_binprm *bprm)
> > > {
> > > 	int ret;
> > > 	char buf[64] = {0};
> > > 
> > > 	ret = bpf_d_path(&bprm->file->f_path, buf, sizeof(buf));
> > > 	if (ret < 0) {
> > > 		bpf_printk("bpf_d_path: ret=%d", ret);
> > > 		return 0;
> > > 	}
> > > 	
> > > 	return 0;
> > > }
> > > ---
> > > 
> > > In this case, the PTR_TO_BTF_ID pointer (&bprm->file->f_path) I
> > > imagine is considered to be trusted and can be passed to the BPF
> > > helper bpf_d_path without the BPF verifier complaining.
> > > 
> > > On the other hand, given the following relatively similar BPF program
> > > as an example:
> > > 
> > > ---
> > > SEC("lsm.s/bprm_committed_creds")
> > > int BPF_PROG(bprm_committed_creds)
> > > {
> > > 	int ret;
> > > 	char buf[64] = {0};
> > > 	struct task_struct *current = bpf_get_current_task_btf();
> > > 
> > > 	ret = bpf_d_path(&current->fs->pwd, buf, sizeof(buf));
> > > 	if (ret < 0) {
> > > 		bpf_printk("bpf_d_path: ret=%d", ret);
> > > 		return 0;
> > > 	}
> > > 	
> > > 	return 0;
> > > }
> > > ---
> > > 
> > > In this case, the PTR_TO_BTF_ID pointer (&current->fs->pwd) is
> > > considered to be untrusted and cannot be passed to the BPF helper
> > > bpf_d_path as the BPF verifier fails to load the BPF program with the
> > > following message:
> > 
> > The reason this is happening is that the struct fs_struct *fs field of
> > struct task_struct is not marked as inheriting its parent task's trusted
> > status. The following diff would fix the issue:
> > 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index fa43dc8e85b9..8b8ccde342f9 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5857,6 +5857,7 @@ BTF_TYPE_SAFE_RCU(struct task_struct) {
> >         struct css_set __rcu *cgroups;
> >         struct task_struct __rcu *real_parent;
> >         struct task_struct *group_leader;
> > + struct fs_struct *fs;
> >  };
> 
> Oh, right. So, if we explicitly dereference the struct fs_struct
> member of struct task_struct within a RCU read-side critical section,
> the BPF verifier considers the pointer to struct fs_struct as being
> safe and trusted. Is that right?

With the above patch, yes.

> Why is it that we need to explicitly add it to such lists so that
> they're considered to be trusted and cannot simply perform the
> bpf_rcu_read_lock/unlock() dance from within the BPF program? Also,
> should we not add the field to BTF_TYPE_SAFE_RCU_OR_NULL() instead of
> BTF_TYPE_SAFE_RCU(), as struct fs_struct could perhaps be NULL in some
> circumstances?

I recommend doing some git log / git blame digging. All of this
information was captured in prior discussions. For example, in the patch
[0] which added these structs.

[0]: https://lore.kernel.org/bpf/20230303041446.3630-7-alexei.starovoitov@gmail.com/

> Are you OK with me carrying this recommended patch to the mailing
> list?

Of course

> >  BTF_TYPE_SAFE_RCU(struct cgroup) {
> > diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_failure.c b/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
> > index dcdea3127086..aef2d4689826 100644
> > --- a/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
> > +++ b/tools/testing/selftests/bpf/progs/task_kfunc_failure.c
> > @@ -324,3 +324,24 @@ int BPF_PROG(task_kfunc_release_in_map, struct task_struct *task, u64 clone_flag
> > 
> >         return 0;
> >  }
> > +
> > +SEC("lsm.s/bprm_committed_creds")
> > +__success
> > +int BPF_PROG(bprm_committed_creds)
> > +{
> > + int ret;
> > + char buf[64] = {0};
> > + struct task_struct *current = bpf_get_current_task_btf();
> > + struct fs_struct *fs;
> > +
> > + bpf_rcu_read_lock();
> > + fs = current->fs;
> > + ret = bpf_d_path(&fs->pwd, buf, sizeof(buf));
> > + bpf_rcu_read_unlock();
> > + if (ret < 0) {
> > +         bpf_printk("bpf_d_path: ret=%d", ret);
> > +         return 0;
> > + }
> > +
> > + return 0;
> > +}
> > 
> > With this patch, the above test would pass (meaning the program is successfully
> > verified).
> > 
> > > 
> > > ---
> > > ; ret = bpf_d_path(&current->fs->pwd, buf, sizeof(buf));
> > > 15: (b7) r3 = 64                      ; R3_w=64
> > > 16: (85) call bpf_d_path#147
> > > R1 type=untrusted_ptr_ expected=ptr_, trusted_ptr_, rcu_ptr_
> > > ---
> > > 
> > > What's interesting is that based on the following commit
> > > (3f00c52393445 bpf: Allow trusted pointers to be passed to
> > > KF_TRUSTED_ARGS kfuncs) message:
> > > 
> > > ---
> > > PTR_TRUSTED pointers are passed directly from the kernel as a
> > > tracepoint or struct_ops callback argument. Any nested pointer that is
> > > obtained from walking a PTR_TRUSTED pointer is no longer
> > > PTR_TRUSTED. From the example above, the struct task_struct *task
> > > argument is PTR_TRUSTED, but the 'nested' pointer obtained from
> > > 'task->last_wakee' is not PTR_TRUSTED.
> > > ---
> > > 
> > > I'm reading this as though the first example program should also fail
> > > BPF verification given that a nested pointer to struct path is
> > > obtained from walking a PTR_TRUSTED pointer, which presumably is
> > > struct linux_binprm in this case. What subtle details am I missing
> > > here? Why is that the first program loads, but the second does not?
> > 
> > The subtle details are in that these are different types. We can't assume that
> > a child pointer automatically inherits its parent's trusted status for all
> > types, so we have to hard code it for now until gcc supports using btf type
> > tags so this can be expressed with annotations like __trusted or __rcu.
> 
> Oh, interesting. You mention that this is explicitly needed for gcc,
> so I'm now wondering how the semantics differ when using clang? Where
> would such annotations (__trusted, __rcu) be applied?

Please see the patch I linked above :-) The point of Alexei's patch was
to keep the same semantics between gcc and clang until gcc finishes
adding support for btf type tags. I recommend reading [1] to learn more
about BTF, if you're unfamiliar.

[1]: https://docs.kernel.org/bpf/btf.html

Once gcc supports BTF type tags, we can apply __trusted, __rcu, etc
annotations directly where the type is defined to encode the information
in BTF, and then use that information in the verifier to inform pointer
safety guarantees. This is already done in many places throughout the
kernel. See e.g. various fields in the definition of struct task_struct.

> > Consider that some types may have NULL child pointers in certain scenarios.
> > Others may be valid as long as you access it in an RCU read region, others may
> > be valid as long as you access it in an RCU read region and it wasn't NULL.
> 
> This makes sense. Thanks for the explanation.
> 
> > The struct linux_binprm type's safety is specified in
> > kernel/bpf/verifier.c:
> > 
> > BTF_TYPE_SAFE_TRUSTED(struct linux_binprm) {
> > 	struct file *file;
> > };
> > 
> > struct task_struct is specified above:
> > 
> > /* RCU trusted: these fields are trusted in RCU CS and never NULL */
> > BTF_TYPE_SAFE_RCU(struct task_struct) {
> >         const cpumask_t *cpus_ptr;
> >         struct css_set __rcu *cgroups;
> >         struct task_struct __rcu *real_parent;
> >         struct task_struct *group_leader;
> > };
> > 
> > > What workaround needs to be implemented in order to have the second
> > > example program satisfy the PTR_TRUSTED contract with the BPF helper
> > > bpf_d_path?
> > 
> > See above, we need to add struct fs_struct * to the list of trusted fields for
> > struct task_struct, and enclose it in an RCU read region.
> 
> /M

Re: BPF/Question: PTR_TRUSTED vs PTR_UNTRUSTED

[Matt Bobrowski]

On Thu, Jul 20, 2023 at 10:16:22AM -0500, David Vernet wrote:
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index fa43dc8e85b9..8b8ccde342f9 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -5857,6 +5857,7 @@ BTF_TYPE_SAFE_RCU(struct task_struct) {
> > >         struct css_set __rcu *cgroups;
> > >         struct task_struct __rcu *real_parent;
> > >         struct task_struct *group_leader;
> > > + struct fs_struct *fs;
> > >  };
> > 
> > Oh, right. So, if we explicitly dereference the struct fs_struct
> > member of struct task_struct within a RCU read-side critical section,
> > the BPF verifier considers the pointer to struct fs_struct as being
> > safe and trusted. Is that right?
> 
> With the above patch, yes.

After conducting some further tests today, it turns out that making
amendments to the struct task_struct BTF_TYPE_SAFE_RCU definition
perhaps isn't actually necessary? As of commit afeebf9f57a49 ("bpf:
Undo strict enforcement for walking untagged fields"), if a trusted
pointer (in this case being struct task_struct obtained via
bpf_get_current_task_btf()) is dereferenced within a RCU read-side
critical section, then the pointer that is yielded as a result of the
walk/dereference operation is a PTR_TO_BTF_ID. It is neither trusted
or untrusted and therefore carries the same level of semantics as a
dereferenced pointer before any trust status for pointers was
introduced within the BPF verifier.

Have I misunderstood something here?

> > Why is it that we need to explicitly add it to such lists so that
> > they're considered to be trusted and cannot simply perform the
> > bpf_rcu_read_lock/unlock() dance from within the BPF program? Also,
> > should we not add the field to BTF_TYPE_SAFE_RCU_OR_NULL() instead of
> > BTF_TYPE_SAFE_RCU(), as struct fs_struct could perhaps be NULL in some
> > circumstances?
> 
> I recommend doing some git log / git blame digging. All of this
> information was captured in prior discussions. For example, in the patch
> [0] which added these structs.
> 
> [0]: https://lore.kernel.org/bpf/20230303041446.3630-7-alexei.starovoitov@gmail.com/
> 
> > Are you OK with me carrying this recommended patch to the mailing
> > list?
> 
> Of course

Based on what I've mentioned above, perhaps sending through a patch no
longer is necessary?

/M

Re: BPF/Question: PTR_TRUSTED vs PTR_UNTRUSTED

[David Vernet]

On Fri, Jul 21, 2023 at 04:44:22PM +0000, Matt Bobrowski wrote:
> On Thu, Jul 20, 2023 at 10:16:22AM -0500, David Vernet wrote:
> > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > index fa43dc8e85b9..8b8ccde342f9 100644
> > > > --- a/kernel/bpf/verifier.c
> > > > +++ b/kernel/bpf/verifier.c
> > > > @@ -5857,6 +5857,7 @@ BTF_TYPE_SAFE_RCU(struct task_struct) {
> > > >         struct css_set __rcu *cgroups;
> > > >         struct task_struct __rcu *real_parent;
> > > >         struct task_struct *group_leader;
> > > > + struct fs_struct *fs;
> > > >  };
> > > 
> > > Oh, right. So, if we explicitly dereference the struct fs_struct
> > > member of struct task_struct within a RCU read-side critical section,
> > > the BPF verifier considers the pointer to struct fs_struct as being
> > > safe and trusted. Is that right?
> > 
> > With the above patch, yes.
> 
> After conducting some further tests today, it turns out that making
> amendments to the struct task_struct BTF_TYPE_SAFE_RCU definition
> perhaps isn't actually necessary? As of commit afeebf9f57a49 ("bpf:
> Undo strict enforcement for walking untagged fields"), if a trusted
> pointer (in this case being struct task_struct obtained via
> bpf_get_current_task_btf()) is dereferenced within a RCU read-side
> critical section, then the pointer that is yielded as a result of the
> walk/dereference operation is a PTR_TO_BTF_ID. It is neither trusted
> or untrusted and therefore carries the same level of semantics as a
> dereferenced pointer before any trust status for pointers was
> introduced within the BPF verifier.
> 
> Have I misunderstood something here?

No, that's correct. You only need the aforementioned patch if you need
the pointer to be a trusted or RCU pointer.

> > > Why is it that we need to explicitly add it to such lists so that
> > > they're considered to be trusted and cannot simply perform the
> > > bpf_rcu_read_lock/unlock() dance from within the BPF program? Also,
> > > should we not add the field to BTF_TYPE_SAFE_RCU_OR_NULL() instead of
> > > BTF_TYPE_SAFE_RCU(), as struct fs_struct could perhaps be NULL in some
> > > circumstances?
> > 
> > I recommend doing some git log / git blame digging. All of this
> > information was captured in prior discussions. For example, in the patch
> > [0] which added these structs.
> > 
> > [0]: https://lore.kernel.org/bpf/20230303041446.3630-7-alexei.starovoitov@gmail.com/
> > 
> > > Are you OK with me carrying this recommended patch to the mailing
> > > list?
> > 
> > Of course
> 
> Based on what I've mentioned above, perhaps sending through a patch no
> longer is necessary?

If you only need to call bpf_d_path() then yes, you shouldn't need the
patch.