From: Daniel Borkmann <daniel@iogearbox.net>
To: KP Singh <kpsingh@chromium.org>,
Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
paulmck@kernel.org, Networking <netdev@vger.kernel.org>,
bpf <bpf@vger.kernel.org>, Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v5 bpf-next 2/5] bpf: Introduce sleepable BPF programs
Date: Wed, 1 Jul 2020 11:34:41 +0200 [thread overview]
Message-ID: <5596445c-7474-9913-6765-5d699c6c5c4e@iogearbox.net> (raw)
In-Reply-To: <CACYkzJ5kGxxA1E70EKah_hWbsb7hoUy8s_Y__uCeSyYxVezaBA@mail.gmail.com>
On 7/1/20 11:15 AM, KP Singh wrote:
> On Wed, Jul 1, 2020 at 1:41 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Wed, Jul 01, 2020 at 01:26:44AM +0200, Daniel Borkmann wrote:
>>> On 6/30/20 6:33 AM, Alexei Starovoitov wrote:
>>> [...]
>>>> +/* list of non-sleepable kernel functions that are otherwise
>>>> + * available to attach by bpf_lsm or fmod_ret progs.
>>>> + */
>>>> +static int check_sleepable_blacklist(unsigned long addr)
>>>> +{
>>>> +#ifdef CONFIG_BPF_LSM
>>>> + if (addr == (long)bpf_lsm_task_free)
>>>> + return -EINVAL;
>>>> +#endif
>>>> +#ifdef CONFIG_SECURITY
>>>> + if (addr == (long)security_task_free)
>>>> + return -EINVAL;
>>>> +#endif
>>>> + return 0;
>>>> +}
>>>
>>> Would be nice to have some sort of generic function annotation to describe
>>> that code cannot sleep inside of it, and then filter based on that. Anyway,
>>> is above from manual code inspection?
>>
>> yep. all manual. I don't think there is a way to automate it.
>> At least I cannot think of one.
>>
>>> What about others like security_sock_rcv_skb() for example which could be
>>> bh_lock_sock()'ed (or, generally hooks running in softirq context)?
>>
>> ahh. it's in running in bh at that point? then it should be added to blacklist.
>>
>> The rough idea I had is to try all lsm_* and security_* hooks with all
>> debug kernel flags and see which ones will complain. Then add them to blacklist.
>> Unfortunately I'm completely swamped and cannot promise to do that
>> in the coming months.
>> So either we wait for somebody to do due diligence or land it knowing
>> that blacklist is incomplete and fix it up one by one.
>> I think the folks who're waiting on sleepable work would prefer the latter.
>> I'm fine whichever way.
>
> Chiming in since I belong to the folks who are waiting on sleepable BPF patches:
>
> 1. Let's obviously add security_sock_rcv_skb to the list.
> 2. I can help in combing through the LSM hooks (at least the comments)
> to look for any other obvious candidates.
> 3. I think it's okay (for us) for this list to be a WIP and build on it with
> proper warnings (in the changelog / comments).
> 4. To make it easier for figuring out which hooks cannot sleep,
> It would be nice if we could:
>
> * Have a helper say, bool bpf_cant_sleep(), available when
> DEBUG_ATOMIC_SLEEP is enabled.
> * Attach LSM programs to all hooks which call this helper and gather data.
> * Let this run on dev machines, run workloads which use the LSM hooks .
>
> 4. Finally, once we do the hard work. We can also think of augmenting the
> LSM_HOOK macro to have structured access to whether a hook is sleepable
> or not (instead of relying on comments).
+1, I think augmenting mid-term would be the best given check_sleepable_blacklist()
is rather a very fragile workaround^hack and it's also a generic lsm/sec hooks issue
(at least for BPF_PROG_TYPE_LSM type & for the sake of documenting it for other LSMs).
Perhaps there are function attributes that could be used and later retrieved via BTF?
Thanks,
Daniel
next prev parent reply other threads:[~2020-07-01 9:34 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-30 4:33 [PATCH v5 bpf-next 0/5] bpf: Introduce minimal support for sleepable progs Alexei Starovoitov
2020-06-30 4:33 ` [PATCH v5 bpf-next 1/5] bpf: Remove redundant synchronize_rcu Alexei Starovoitov
2020-06-30 20:55 ` Paul E. McKenney
2020-06-30 4:33 ` [PATCH v5 bpf-next 2/5] bpf: Introduce sleepable BPF programs Alexei Starovoitov
2020-06-30 22:04 ` Paul E. McKenney
2020-06-30 23:26 ` Daniel Borkmann
2020-06-30 23:41 ` Alexei Starovoitov
2020-07-01 9:15 ` KP Singh
2020-07-01 9:34 ` Daniel Borkmann [this message]
2020-07-01 15:21 ` Alexei Starovoitov
2020-07-10 17:00 ` Arnaldo Carvalho de Melo
2020-07-10 22:59 ` Alexei Starovoitov
2020-07-01 9:17 ` Daniel Borkmann
2020-07-01 15:14 ` Alexei Starovoitov
2020-07-01 19:03 ` KP Singh
2020-06-30 4:33 ` [PATCH v5 bpf-next 3/5] bpf: Add bpf_copy_from_user() helper Alexei Starovoitov
2020-06-30 11:41 ` KP Singh
2020-06-30 4:33 ` [PATCH v5 bpf-next 4/5] libbpf: support sleepable progs Alexei Starovoitov
2020-06-30 4:33 ` [PATCH v5 bpf-next 5/5] selftests/bpf: Add sleepable tests Alexei Starovoitov
2020-06-30 5:14 ` [PATCH v5 bpf-next 0/5] bpf: Introduce minimal support for sleepable progs Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5596445c-7474-9913-6765-5d699c6c5c4e@iogearbox.net \
--to=daniel@iogearbox.net \
--cc=alexei.starovoitov@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=davem@davemloft.net \
--cc=kernel-team@fb.com \
--cc=kpsingh@chromium.org \
--cc=netdev@vger.kernel.org \
--cc=paulmck@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).