BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Martin KaFai Lau <kafai@fb.com>
To: KP Singh <kpsingh@chromium.org>
Cc: <linux-kernel@vger.kernel.org>, <bpf@vger.kernel.org>,
	<linux-security-module@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Paul Turner <pjt@google.com>, Jann Horn <jannh@google.com>,
	Florent Revest <revest@chromium.org>
Subject: Re: [PATCH bpf-next v6 3/7] bpf: Generalize bpf_sk_storage
Date: Fri, 24 Jul 2020 18:13:29 -0700
Message-ID: <20200725011329.ymvhmbb2y5yqzy3k@kafai-mbp> (raw)
In-Reply-To: <20200723115032.460770-4-kpsingh@chromium.org>

On Thu, Jul 23, 2020 at 01:50:28PM +0200, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> Refactor the functionality in bpf_sk_storage.c so that concept of
> storage linked to kernel objects can be extended to other objects like
> inode, task_struct etc.
> 
> Each new local storage will still be a separate map and provide its own
> set of helpers. This allows for future object specific extensions and
> still share a lot of the underlying implementation.
> 

[ ... ]

> @@ -386,54 +407,28 @@ static int sk_storage_alloc(struct sock *sk,
>   * Otherwise, it will become a leak (and other memory issues
>   * during map destruction).
>   */
> -static struct bpf_local_storage_data *
> -bpf_local_storage_update(struct sock *sk, struct bpf_map *map, void *value,
> +struct bpf_local_storage_data *
> +bpf_local_storage_update(void *owner, struct bpf_map *map,
> +			 struct bpf_local_storage *local_storage, void *value,
>  			 u64 map_flags)
>  {
>  	struct bpf_local_storage_data *old_sdata = NULL;
>  	struct bpf_local_storage_elem *selem;
> -	struct bpf_local_storage *local_storage;
>  	struct bpf_local_storage_map *smap;
>  	int err;
>  
> -	/* BPF_EXIST and BPF_NOEXIST cannot be both set */
> -	if (unlikely((map_flags & ~BPF_F_LOCK) > BPF_EXIST) ||
> -	    /* BPF_F_LOCK can only be used in a value with spin_lock */
> -	    unlikely((map_flags & BPF_F_LOCK) && !map_value_has_spin_lock(map)))
> -		return ERR_PTR(-EINVAL);
> -
>  	smap = (struct bpf_local_storage_map *)map;
> -	local_storage = rcu_dereference(sk->sk_bpf_storage);
> -	if (!local_storage || hlist_empty(&local_storage->list)) {
> -		/* Very first elem for this object */
> -		err = check_flags(NULL, map_flags);
This check_flags here is missing in the later sk_storage_update().

> -		if (err)
> -			return ERR_PTR(err);
> -
> -		selem = bpf_selem_alloc(smap, sk, value, true);
> -		if (!selem)
> -			return ERR_PTR(-ENOMEM);
> -
> -		err = sk_storage_alloc(sk, smap, selem);
> -		if (err) {
> -			kfree(selem);
> -			atomic_sub(smap->elem_size, &sk->sk_omem_alloc);
> -			return ERR_PTR(err);
> -		}
> -
> -		return SDATA(selem);
> -	}
>  
>  	if ((map_flags & BPF_F_LOCK) && !(map_flags & BPF_NOEXIST)) {
>  		/* Hoping to find an old_sdata to do inline update
>  		 * such that it can avoid taking the local_storage->lock
>  		 * and changing the lists.
>  		 */
> -		old_sdata =
> -			bpf_local_storage_lookup(local_storage, smap, false);
> +		old_sdata = bpf_local_storage_lookup(local_storage, smap, false);
>  		err = check_flags(old_sdata, map_flags);
>  		if (err)
>  			return ERR_PTR(err);
> +
>  		if (old_sdata && selem_linked_to_storage(SELEM(old_sdata))) {
>  			copy_map_value_locked(map, old_sdata->data,
>  					      value, false);

[ ... ]

> +static struct bpf_local_storage_data *
> +sk_storage_update(void *owner, struct bpf_map *map, void *value, u64 map_flags)
> +{
> +	struct bpf_local_storage_data *old_sdata = NULL;
> +	struct bpf_local_storage_elem *selem;
> +	struct bpf_local_storage *local_storage;
> +	struct bpf_local_storage_map *smap;
> +	struct sock *sk;
> +	int err;
> +
> +	err = bpf_local_storage_check_update_flags(map, map_flags);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	sk = owner;
> +	local_storage = rcu_dereference(sk->sk_bpf_storage);
> +	smap = (struct bpf_local_storage_map *)map;
> +
> +	if (!local_storage || hlist_empty(&local_storage->list)) {

"check_flags(NULL, map_flags);" is gone in this refactoring.

This part of code is copied into the inode_storage_update()
in the latter patch which then has the same issue.

> +		/* Very first elem */
> +		selem = map->ops->map_selem_alloc(smap, owner, value, !old_sdata);
> +		if (!selem)
> +			return ERR_PTR(-ENOMEM);

>  static int sk_storage_map_btf_id;
>  const struct bpf_map_ops sk_storage_map_ops = {
> -	.map_alloc_check = bpf_sk_storage_map_alloc_check,
> -	.map_alloc = bpf_local_storage_map_alloc,
> -	.map_free = bpf_local_storage_map_free,
> +	.map_alloc_check = bpf_local_storage_map_alloc_check,
> +	.map_alloc = sk_storage_map_alloc,
> +	.map_free = sk_storage_map_free,
>  	.map_get_next_key = notsupp_get_next_key,
>  	.map_lookup_elem = bpf_fd_sk_storage_lookup_elem,
>  	.map_update_elem = bpf_fd_sk_storage_update_elem,
>  	.map_delete_elem = bpf_fd_sk_storage_delete_elem,
> -	.map_check_btf = bpf_sk_storage_map_check_btf,
> +	.map_check_btf = bpf_local_storage_map_check_btf,
>  	.map_btf_name = "bpf_local_storage_map",
>  	.map_btf_id = &sk_storage_map_btf_id,
> +	.map_selem_alloc = sk_selem_alloc,
> +	.map_local_storage_update = sk_storage_update,
> +	.map_local_storage_unlink = unlink_sk_storage,
I think refactoring codes as map_selem_alloc, map_local_storage_update,
and map_local_storage_unlink is not the best option.  The sk and inode
version of the above map_ops are mostly the same.  Fixing the
issue like the one mentioned above need to fix both sk, inode, and
the future kernel-object code.

The only difference is sk charge omem and inode does not.
I have played around a little.  I think adding the following three ops (pasted at
the end) is better and should be enough for both sk and inode.  The inode
does not even have to implement the (un)charge ops at all.

That should remove the duplication for the followings:
- (sk|inode)_selem_alloc
- (sk|inode)_storage_update
- unlink_(sk|inode)_storage
- (sk|inode)_storage_alloc

Another bonus is the new bpf_local_storage_check_update_flags() and
bpf_local_storage_publish() will be no longer needed too.

I have hacked up this patch 3 change to compiler-test out this idea.
I will post in another email.  Let me know wdyt.

--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -33,6 +33,9 @@ struct btf;
 struct btf_type;
 struct exception_table_entry;
 struct seq_operations;
+struct bpf_local_storage;
+struct bpf_local_storage_map;
+struct bpf_local_storage_elem;
 
 extern struct idr btf_idr;
 extern spinlock_t btf_idr_lock;
@@ -93,6 +96,13 @@ struct bpf_map_ops {
 	__poll_t (*map_poll)(struct bpf_map *map, struct file *filp,
 			     struct poll_table_struct *pts);
 
+	/* Functions called by bpf_local_storage maps */
+	int (*map_local_storage_charge)(struct bpf_local_storage_map *smap,
+					void *owner, u32 size);
+	void (*map_local_storage_uncharge)(struct bpf_local_storage_map *smap,
+					   void *owner, u32 size);
+	struct bpf_local_storage __rcu ** (*map_owner_storage_ptr)(struct bpf_local_storage_map *smap,
+								   void *owner);
 	/* BTF name and id of struct allocated by map_alloc */
 	const char * const map_btf_name;
 	int *map_btf_id;

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-23 11:50 [PATCH bpf-next v6 0/7] Generalizing bpf_local_storage KP Singh
2020-07-23 11:50 ` [PATCH bpf-next v6 1/7] bpf: Renames to prepare for generalizing sk_storage KP Singh
2020-07-24  5:31   ` Martin KaFai Lau
2020-07-24 15:44     ` KP Singh
2020-07-23 11:50 ` [PATCH bpf-next v6 2/7] bpf: Generalize caching for sk_storage KP Singh
2020-07-23 11:50 ` [PATCH bpf-next v6 3/7] bpf: Generalize bpf_sk_storage KP Singh
2020-07-25  1:13   ` Martin KaFai Lau [this message]
2020-07-27 20:28     ` KP Singh
2020-07-25  1:30   ` [RFC PATCH bpf-next] bpf: POC on local_storage charge and uncharge map_ops Martin KaFai Lau
2020-07-27 20:26     ` KP Singh
2020-07-27 21:43       ` Martin KaFai Lau
2020-07-23 11:50 ` [PATCH bpf-next v6 4/7] bpf: Split bpf_local_storage to bpf_sk_storage KP Singh
2020-07-23 11:50 ` [PATCH bpf-next v6 5/7] bpf: Implement bpf_local_storage for inodes KP Singh
2020-07-23 11:50 ` [PATCH bpf-next v6 6/7] bpf: Allow local storage to be used from LSM programs KP Singh
2020-07-23 11:50 ` [PATCH bpf-next v6 7/7] bpf: Add selftests for local_storage KP Singh

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=20200725011329.ymvhmbb2y5yqzy3k@kafai-mbp \
    --to=kafai@fb.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=jannh@google.com \
    --cc=kpsingh@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=pjt@google.com \
    --cc=revest@chromium.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

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git