On Fri, Jan 08, 2021 at 03:19:47PM -0800, Song Liu wrote:
[ ... ]
> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> index dd5aedee99e73..9bd47ad2b26f1 100644
> --- a/kernel/bpf/bpf_local_storage.c
> +++ b/kernel/bpf/bpf_local_storage.c
> @@ -140,17 +140,18 @@ static void __bpf_selem_unlink_storage(struct bpf_local_storage_elem *selem)
> {
> struct bpf_local_storage *local_storage;
> bool free_local_storage = false;
> + unsigned long flags;
>
> if (unlikely(!selem_linked_to_storage(selem)))
> /* selem has already been unlinked from sk */
> return;
>
> local_storage = rcu_dereference(selem->local_storage);
> - raw_spin_lock_bh(&local_storage->lock);
> + raw_spin_lock_irqsave(&local_storage->lock, flags);
It will be useful to have a few words in commit message on this change
for future reference purpose.
Please also remove the in_irq() check from bpf_sk_storage.c
to avoid confusion in the future. It probably should
be in a separate patch.
[ ... ]
> diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
> index 4ef1959a78f27..f654b56907b69 100644
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 7425b3224891d..3d65c8ebfd594 100644
[ ... ]
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -96,6 +96,7 @@
> #include <linux/kasan.h>
> #include <linux/scs.h>
> #include <linux/io_uring.h>
> +#include <linux/bpf.h>
>
> #include <asm/pgalloc.h>
> #include <linux/uaccess.h>
> @@ -734,6 +735,7 @@ void __put_task_struct(struct task_struct *tsk)
> cgroup_free(tsk);
> task_numa_free(tsk, true);
> security_task_free(tsk);
> + bpf_task_storage_free(tsk);
> exit_creds(tsk);
If exit_creds() is traced by a bpf and this bpf is doing
bpf_task_storage_get(..., BPF_LOCAL_STORAGE_GET_F_CREATE),
new task storage will be created after bpf_task_storage_free().
I recalled there was an earlier discussion with KP and KP mentioned
BPF_LSM will not be called with a task that is going away.
It seems enabling bpf task storage in bpf tracing will break
this assumption and needs to be addressed?
On Mon, Jan 11, 2021 at 7:57 PM Martin KaFai Lau <[email protected]> wrote:
>
> On Fri, Jan 08, 2021 at 03:19:47PM -0800, Song Liu wrote:
>
> [ ... ]
>
> > diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> > index dd5aedee99e73..9bd47ad2b26f1 100644
> > --- a/kernel/bpf/bpf_local_storage.c
> > +++ b/kernel/bpf/bpf_local_storage.c
> > @@ -140,17 +140,18 @@ static void __bpf_selem_unlink_storage(struct bpf_local_storage_elem *selem)
> > {
> > struct bpf_local_storage *local_storage;
> > bool free_local_storage = false;
> > + unsigned long flags;
> >
> > if (unlikely(!selem_linked_to_storage(selem)))
> > /* selem has already been unlinked from sk */
> > return;
> >
> > local_storage = rcu_dereference(selem->local_storage);
> > - raw_spin_lock_bh(&local_storage->lock);
> > + raw_spin_lock_irqsave(&local_storage->lock, flags);
> It will be useful to have a few words in commit message on this change
> for future reference purpose.
>
> Please also remove the in_irq() check from bpf_sk_storage.c
> to avoid confusion in the future. It probably should
> be in a separate patch.
>
> [ ... ]
>
> > diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
> > index 4ef1959a78f27..f654b56907b69 100644
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 7425b3224891d..3d65c8ebfd594 100644
> [ ... ]
>
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -96,6 +96,7 @@
> > #include <linux/kasan.h>
> > #include <linux/scs.h>
> > #include <linux/io_uring.h>
> > +#include <linux/bpf.h>
> >
> > #include <asm/pgalloc.h>
> > #include <linux/uaccess.h>
> > @@ -734,6 +735,7 @@ void __put_task_struct(struct task_struct *tsk)
> > cgroup_free(tsk);
> > task_numa_free(tsk, true);
> > security_task_free(tsk);
> > + bpf_task_storage_free(tsk);
> > exit_creds(tsk);
> If exit_creds() is traced by a bpf and this bpf is doing
> bpf_task_storage_get(..., BPF_LOCAL_STORAGE_GET_F_CREATE),
> new task storage will be created after bpf_task_storage_free().
>
> I recalled there was an earlier discussion with KP and KP mentioned
> BPF_LSM will not be called with a task that is going away.
> It seems enabling bpf task storage in bpf tracing will break
> this assumption and needs to be addressed?
For tracing programs, I think we will need an allow list where
task local storage can be used.
On Mon, Jan 11, 2021 at 10:35:43PM +0100, KP Singh wrote:
> On Mon, Jan 11, 2021 at 7:57 PM Martin KaFai Lau <[email protected]> wrote:
> >
> > On Fri, Jan 08, 2021 at 03:19:47PM -0800, Song Liu wrote:
> >
> > [ ... ]
> >
> > > diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> > > index dd5aedee99e73..9bd47ad2b26f1 100644
> > > --- a/kernel/bpf/bpf_local_storage.c
> > > +++ b/kernel/bpf/bpf_local_storage.c
> > > @@ -140,17 +140,18 @@ static void __bpf_selem_unlink_storage(struct bpf_local_storage_elem *selem)
> > > {
> > > struct bpf_local_storage *local_storage;
> > > bool free_local_storage = false;
> > > + unsigned long flags;
> > >
> > > if (unlikely(!selem_linked_to_storage(selem)))
> > > /* selem has already been unlinked from sk */
> > > return;
> > >
> > > local_storage = rcu_dereference(selem->local_storage);
> > > - raw_spin_lock_bh(&local_storage->lock);
> > > + raw_spin_lock_irqsave(&local_storage->lock, flags);
> > It will be useful to have a few words in commit message on this change
> > for future reference purpose.
> >
> > Please also remove the in_irq() check from bpf_sk_storage.c
> > to avoid confusion in the future. It probably should
> > be in a separate patch.
> >
> > [ ... ]
> >
> > > diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
> > > index 4ef1959a78f27..f654b56907b69 100644
> > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > index 7425b3224891d..3d65c8ebfd594 100644
> > [ ... ]
> >
> > > --- a/kernel/fork.c
> > > +++ b/kernel/fork.c
> > > @@ -96,6 +96,7 @@
> > > #include <linux/kasan.h>
> > > #include <linux/scs.h>
> > > #include <linux/io_uring.h>
> > > +#include <linux/bpf.h>
> > >
> > > #include <asm/pgalloc.h>
> > > #include <linux/uaccess.h>
> > > @@ -734,6 +735,7 @@ void __put_task_struct(struct task_struct *tsk)
> > > cgroup_free(tsk);
> > > task_numa_free(tsk, true);
> > > security_task_free(tsk);
> > > + bpf_task_storage_free(tsk);
> > > exit_creds(tsk);
> > If exit_creds() is traced by a bpf and this bpf is doing
> > bpf_task_storage_get(..., BPF_LOCAL_STORAGE_GET_F_CREATE),
> > new task storage will be created after bpf_task_storage_free().
> >
> > I recalled there was an earlier discussion with KP and KP mentioned
> > BPF_LSM will not be called with a task that is going away.
> > It seems enabling bpf task storage in bpf tracing will break
> > this assumption and needs to be addressed?
>
> For tracing programs, I think we will need an allow list where
> task local storage can be used.
Instead of whitelist, can refcount_inc_not_zero(&tsk->usage) be used?
> On Jan 11, 2021, at 10:56 AM, Martin Lau <[email protected]> wrote:
>
> On Fri, Jan 08, 2021 at 03:19:47PM -0800, Song Liu wrote:
>
> [ ... ]
>
>> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
>> index dd5aedee99e73..9bd47ad2b26f1 100644
>> --- a/kernel/bpf/bpf_local_storage.c
>> +++ b/kernel/bpf/bpf_local_storage.c
>> @@ -140,17 +140,18 @@ static void __bpf_selem_unlink_storage(struct bpf_local_storage_elem *selem)
>> {
>> struct bpf_local_storage *local_storage;
>> bool free_local_storage = false;
>> + unsigned long flags;
>>
>> if (unlikely(!selem_linked_to_storage(selem)))
>> /* selem has already been unlinked from sk */
>> return;
>>
>> local_storage = rcu_dereference(selem->local_storage);
>> - raw_spin_lock_bh(&local_storage->lock);
>> + raw_spin_lock_irqsave(&local_storage->lock, flags);
> It will be useful to have a few words in commit message on this change
> for future reference purpose.
>
> Please also remove the in_irq() check from bpf_sk_storage.c
> to avoid confusion in the future. It probably should
> be in a separate patch.
Do you mean we allow bpf_sk_storage_get_tracing() and
bpf_sk_storage_delete_tracing() in irq context? Like
diff --git i/net/core/bpf_sk_storage.c w/net/core/bpf_sk_storage.c
index 4edd033e899c0..14dd5e3c67402 100644
--- i/net/core/bpf_sk_storage.c
+++ w/net/core/bpf_sk_storage.c
@@ -425,7 +425,7 @@ BPF_CALL_4(bpf_sk_storage_get_tracing, struct bpf_map *, map, struct sock *, sk,
BPF_CALL_2(bpf_sk_storage_delete_tracing, struct bpf_map *, map,
struct sock *, sk)
{
- if (in_irq() || in_nmi())
+ if (in_nmi())
return -EPERM;
return ____bpf_sk_storage_delete(map, sk);
[...]
On Mon, Jan 11, 2021 at 03:41:26PM -0800, Song Liu wrote:
>
>
> > On Jan 11, 2021, at 10:56 AM, Martin Lau <[email protected]> wrote:
> >
> > On Fri, Jan 08, 2021 at 03:19:47PM -0800, Song Liu wrote:
> >
> > [ ... ]
> >
> >> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> >> index dd5aedee99e73..9bd47ad2b26f1 100644
> >> --- a/kernel/bpf/bpf_local_storage.c
> >> +++ b/kernel/bpf/bpf_local_storage.c
> >> @@ -140,17 +140,18 @@ static void __bpf_selem_unlink_storage(struct bpf_local_storage_elem *selem)
> >> {
> >> struct bpf_local_storage *local_storage;
> >> bool free_local_storage = false;
> >> + unsigned long flags;
> >>
> >> if (unlikely(!selem_linked_to_storage(selem)))
> >> /* selem has already been unlinked from sk */
> >> return;
> >>
> >> local_storage = rcu_dereference(selem->local_storage);
> >> - raw_spin_lock_bh(&local_storage->lock);
> >> + raw_spin_lock_irqsave(&local_storage->lock, flags);
> > It will be useful to have a few words in commit message on this change
> > for future reference purpose.
> >
> > Please also remove the in_irq() check from bpf_sk_storage.c
> > to avoid confusion in the future. It probably should
> > be in a separate patch.
>
> Do you mean we allow bpf_sk_storage_get_tracing() and
> bpf_sk_storage_delete_tracing() in irq context? Like
Right.
However, after another thought, may be lets skip that for now
till a use case comes up and a test can be written.
> On Jan 11, 2021, at 1:58 PM, Martin Lau <[email protected]> wrote:
>
> On Mon, Jan 11, 2021 at 10:35:43PM +0100, KP Singh wrote:
>> On Mon, Jan 11, 2021 at 7:57 PM Martin KaFai Lau <[email protected]> wrote:
>>>
>>> On Fri, Jan 08, 2021 at 03:19:47PM -0800, Song Liu wrote:
>>>
>>> [ ... ]
>>>
>>>> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
>>>> index dd5aedee99e73..9bd47ad2b26f1 100644
>>>> --- a/kernel/bpf/bpf_local_storage.c
>>>> +++ b/kernel/bpf/bpf_local_storage.c
>>>> @@ -140,17 +140,18 @@ static void __bpf_selem_unlink_storage(struct bpf_local_storage_elem *selem)
>>>> {
>>>> struct bpf_local_storage *local_storage;
>>>> bool free_local_storage = false;
>>>> + unsigned long flags;
>>>>
>>>> if (unlikely(!selem_linked_to_storage(selem)))
>>>> /* selem has already been unlinked from sk */
>>>> return;
>>>>
>>>> local_storage = rcu_dereference(selem->local_storage);
>>>> - raw_spin_lock_bh(&local_storage->lock);
>>>> + raw_spin_lock_irqsave(&local_storage->lock, flags);
>>> It will be useful to have a few words in commit message on this change
>>> for future reference purpose.
>>>
>>> Please also remove the in_irq() check from bpf_sk_storage.c
>>> to avoid confusion in the future. It probably should
>>> be in a separate patch.
>>>
>>> [ ... ]
>>>
>>>> diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
>>>> index 4ef1959a78f27..f654b56907b69 100644
>>>> diff --git a/kernel/fork.c b/kernel/fork.c
>>>> index 7425b3224891d..3d65c8ebfd594 100644
>>> [ ... ]
>>>
>>>> --- a/kernel/fork.c
>>>> +++ b/kernel/fork.c
>>>> @@ -96,6 +96,7 @@
>>>> #include <linux/kasan.h>
>>>> #include <linux/scs.h>
>>>> #include <linux/io_uring.h>
>>>> +#include <linux/bpf.h>
>>>>
>>>> #include <asm/pgalloc.h>
>>>> #include <linux/uaccess.h>
>>>> @@ -734,6 +735,7 @@ void __put_task_struct(struct task_struct *tsk)
>>>> cgroup_free(tsk);
>>>> task_numa_free(tsk, true);
>>>> security_task_free(tsk);
>>>> + bpf_task_storage_free(tsk);
>>>> exit_creds(tsk);
>>> If exit_creds() is traced by a bpf and this bpf is doing
>>> bpf_task_storage_get(..., BPF_LOCAL_STORAGE_GET_F_CREATE),
>>> new task storage will be created after bpf_task_storage_free().
>>>
>>> I recalled there was an earlier discussion with KP and KP mentioned
>>> BPF_LSM will not be called with a task that is going away.
>>> It seems enabling bpf task storage in bpf tracing will break
>>> this assumption and needs to be addressed?
>>
>> For tracing programs, I think we will need an allow list where
>> task local storage can be used.
> Instead of whitelist, can refcount_inc_not_zero(&tsk->usage) be used?
I think we can put refcount_inc_not_zero() in bpf_task_storage_get, like:
diff --git i/kernel/bpf/bpf_task_storage.c w/kernel/bpf/bpf_task_storage.c
index f654b56907b69..93d01b0a010e6 100644
--- i/kernel/bpf/bpf_task_storage.c
+++ w/kernel/bpf/bpf_task_storage.c
@@ -216,6 +216,9 @@ BPF_CALL_4(bpf_task_storage_get, struct bpf_map *, map, struct task_struct *,
* by an RCU read-side critical section.
*/
if (flags & BPF_LOCAL_STORAGE_GET_F_CREATE) {
+ if (!refcount_inc_not_zero(&task->usage))
+ return -EBUSY;
+
sdata = bpf_local_storage_update(
task, (struct bpf_local_storage_map *)map, value,
BPF_NOEXIST);
But where shall we add the refcount_dec()? IIUC, we cannot add it to
__put_task_struct().
Thanks,
Song