All of lore.kernel.org
 help / color / mirror / Atom feed
From: John Fastabend <john.fastabend@gmail.com>
To: Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	ast@kernel.org, daniel@iogearbox.net
Cc: lmb@cloudflare.com, bpf@vger.kernel.org, jakub@cloudflare.com,
	netdev@vger.kernel.org
Subject: Re: [bpf-next PATCH 2/3] bpf: sk_msg helpers for probe_* and *current_task*
Date: Thu, 14 May 2020 06:30:55 -0700	[thread overview]
Message-ID: <5ebd480fa503a_7f582b1a484825b47a@john-XPS-13-9370.notmuch> (raw)
In-Reply-To: <35846427-3770-f6ab-b1a6-c974a835f746@fb.com>

Yonghong Song wrote:
> 
> 
> On 5/13/20 12:24 PM, John Fastabend wrote:
> > Often it is useful when applying policy to know something about the
> > task. If the administrator has CAP_SYS_ADMIN rights then they can
> > use kprobe + sk_msg and link the two programs together to accomplish
> > this. However, this is a bit clunky and also means we have to call
> > sk_msg program and kprobe program when we could just use a single
> > program and avoid passing metadata through sk_msg/skb, socket, etc.
> > 
> > To accomplish this add probe_* helpers to sk_msg programs guarded
> > by a CAP_SYS_ADMIN check. New supported helpers are the following,
> > 
> >   BPF_FUNC_get_current_task
> >   BPF_FUNC_current_task_under_cgroup
> >   BPF_FUNC_probe_read_user
> >   BPF_FUNC_probe_read_kernel
> >   BPF_FUNC_probe_read
> >   BPF_FUNC_probe_read_user_str
> >   BPF_FUNC_probe_read_kernel_str
> >   BPF_FUNC_probe_read_str
> 
> I think this is a good idea. But this will require bpf program
> to be GPLed, probably it will be okay. Currently, for capabilities,
> it is CAP_SYS_ADMIN now, in the future, it may be CAP_PERFMON.

Right.

> 
> Also, do we want to remove BPF_FUNC_probe_read and
> BPF_FUNC_probe_read_str from the list? Since we
> introduce helpers to new program types, we can deprecate
> these two helpers right away.

Removed, Daniel had the same comment.

> 
> The new helpers will be subject to new security lockdown
> rules which may have impact on networking bpf programs
> on particular setup.

But only if these helpers are used. If not everything should
be the same I think.

> 
> > 
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > ---

[...]

> > @@ -6397,6 +6406,31 @@ sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >   		return &bpf_get_cgroup_classid_curr_proto;
> >   #endif
> >   	default:
> > +		break;
> > +	}
> > +
> > +	if (!capable(CAP_SYS_ADMIN))
> > +		return bpf_base_func_proto(func_id);
> > +
> > +	/* All helpers below are for CAP_SYS_ADMIN only */
> > +	switch (func_id) {
> > +	case BPF_FUNC_get_current_task:
> > +		return &bpf_get_current_task_proto;
> > +	case BPF_FUNC_current_task_under_cgroup:
> > +		return &bpf_current_task_under_cgroup_proto;
> > +	case BPF_FUNC_probe_read_user:
> > +		return &bpf_probe_read_user_proto;
> > +	case BPF_FUNC_probe_read_kernel:
> > +		return &bpf_probe_read_kernel_proto;
> > +	case BPF_FUNC_probe_read:
> > +		return &bpf_probe_read_compat_proto;
> > +	case BPF_FUNC_probe_read_user_str:
> > +		return &bpf_probe_read_user_str_proto;
> > +	case BPF_FUNC_probe_read_kernel_str:
> > +		return &bpf_probe_read_kernel_str_proto;
> > +	case BPF_FUNC_probe_read_str:
> > +		return &bpf_probe_read_compat_str_proto;
> > +	default:
> >   		return bpf_base_func_proto(func_id);
> 
> If we can get a consensus here, I think we can even folding all
> these bpf helpers (get_current_task, ..., probe_read_kernel_str)
> to bpf_base_func_proto, so any bpf program types including
> other networking types can use them.
> Any concerns?
> 

Nothing comes to mind. I'm OK to move them into base if folks
agree its useful there. I was putting them where I have a known
use case at the moment but doesn't bother me to make them more
widely available.

  reply	other threads:[~2020-05-14 13:31 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13 19:23 [bpf-next PATCH 0/3] bpf: Add sk_msg helpers John Fastabend
2020-05-13 19:23 ` [bpf-next PATCH 1/3] bpf: sk_msg add some generic helpers that may be useful from sk_msg John Fastabend
2020-05-14  6:58   ` Yonghong Song
2020-05-13 19:24 ` [bpf-next PATCH 2/3] bpf: sk_msg helpers for probe_* and *current_task* John Fastabend
2020-05-14  7:21   ` Yonghong Song
2020-05-14 13:30     ` John Fastabend [this message]
2020-05-14  8:02   ` Daniel Borkmann
2020-05-13 19:24 ` [bpf-next PATCH 3/3] bpf: sk_msg add get socket storage helpers John Fastabend
2020-05-14  7:26   ` Yonghong Song

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=5ebd480fa503a_7f582b1a484825b47a@john-XPS-13-9370.notmuch \
    --to=john.fastabend@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=jakub@cloudflare.com \
    --cc=lmb@cloudflare.com \
    --cc=netdev@vger.kernel.org \
    --cc=yhs@fb.com \
    --subject='Re: [bpf-next PATCH 2/3] bpf: sk_msg helpers for probe_* and *current_task*' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.