Re: [PATCH net-next v12 14/15] p4tc: add set of P4TC table kfuncs

[Martin KaFai Lau <martin.lau@linux.dev>]

On 2/25/24 8:54 AM, Jamal Hadi Salim wrote:
> +struct p4tc_table_entry_act_bpf_params {

Will this struct be extended in the future?

> +	u32 pipeid;
> +	u32 tblid;
> +};
> +
> +struct p4tc_table_entry_create_bpf_params {
> +	u32 profile_id;
> +	u32 pipeid;
> +	u32 tblid;
> +};
> +

[ ... ]

> diff --git a/include/net/tc_act/p4tc.h b/include/net/tc_act/p4tc.h
> index c5256d821..155068de0 100644
> --- a/include/net/tc_act/p4tc.h
> +++ b/include/net/tc_act/p4tc.h
> @@ -13,10 +13,26 @@ struct tcf_p4act_params {
>   	u32 tot_params_sz;
>   };
>   
> +#define P4TC_MAX_PARAM_DATA_SIZE 124
> +
> +struct p4tc_table_entry_act_bpf {
> +	u32 act_id;
> +	u32 hit:1,
> +	    is_default_miss_act:1,
> +	    is_default_hit_act:1;
> +	u8 params[P4TC_MAX_PARAM_DATA_SIZE];
> +} __packed;
> +
> +struct p4tc_table_entry_act_bpf_kern {
> +	struct rcu_head rcu;
> +	struct p4tc_table_entry_act_bpf act_bpf;
> +};
> +
>   struct tcf_p4act {
>   	struct tc_action common;
>   	/* Params IDR reference passed during runtime */
>   	struct tcf_p4act_params __rcu *params;
> +	struct p4tc_table_entry_act_bpf_kern __rcu *act_bpf;
>   	u32 p_id;
>   	u32 act_id;
>   	struct list_head node;
> @@ -24,4 +40,39 @@ struct tcf_p4act {
>   
>   #define to_p4act(a) ((struct tcf_p4act *)a)
>   
> +static inline struct p4tc_table_entry_act_bpf *
> +p4tc_table_entry_act_bpf(struct tc_action *action)
> +{
> +	struct p4tc_table_entry_act_bpf_kern *act_bpf;
> +	struct tcf_p4act *p4act = to_p4act(action);
> +
> +	act_bpf = rcu_dereference(p4act->act_bpf);
> +
> +	return &act_bpf->act_bpf;
> +}
> +
> +static inline int
> +p4tc_table_entry_act_bpf_change_flags(struct tc_action *action, u32 hit,
> +				      u32 dflt_miss, u32 dflt_hit)
> +{
> +	struct p4tc_table_entry_act_bpf_kern *act_bpf, *act_bpf_old;
> +	struct tcf_p4act *p4act = to_p4act(action);
> +
> +	act_bpf = kzalloc(sizeof(*act_bpf), GFP_KERNEL);


[ ... ]

> +__bpf_kfunc static struct p4tc_table_entry_act_bpf *
> +bpf_p4tc_tbl_read(struct __sk_buff *skb_ctx,

The argument could be "struct sk_buff *skb" instead of __sk_buff. Take a look at 
commit 2f4643934670.

> +		  struct p4tc_table_entry_act_bpf_params *params,
> +		  void *key, const u32 key__sz)
> +{
> +	struct sk_buff *skb = (struct sk_buff *)skb_ctx;
> +	struct net *caller_net;
> +
> +	caller_net = skb->dev ? dev_net(skb->dev) : sock_net(skb->sk);
> +
> +	return __bpf_p4tc_tbl_read(caller_net, params, key, key__sz);
> +}
> +
> +__bpf_kfunc static struct p4tc_table_entry_act_bpf *
> +xdp_p4tc_tbl_read(struct xdp_md *xdp_ctx,
> +		  struct p4tc_table_entry_act_bpf_params *params,
> +		  void *key, const u32 key__sz)
> +{
> +	struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
> +	struct net *caller_net;
> +
> +	caller_net = dev_net(ctx->rxq->dev);
> +
> +	return __bpf_p4tc_tbl_read(caller_net, params, key, key__sz);
> +}
> +
> +static int
> +__bpf_p4tc_entry_create(struct net *net,
> +			struct p4tc_table_entry_create_bpf_params *params,
> +			void *key, const u32 key__sz,
> +			struct p4tc_table_entry_act_bpf *act_bpf)
> +{
> +	struct p4tc_table_entry_key *entry_key = key;
> +	struct p4tc_pipeline *pipeline;
> +	struct p4tc_table *table;
> +
> +	if (!params || !key)
> +		return -EINVAL;
> +	if (key__sz != P4TC_ENTRY_KEY_SZ_BYTES(entry_key->keysz))
> +		return -EINVAL;
> +
> +	pipeline = p4tc_pipeline_find_byid(net, params->pipeid);
> +	if (!pipeline)
> +		return -ENOENT;
> +
> +	table = p4tc_tbl_cache_lookup(net, params->pipeid, params->tblid);
> +	if (!table)
> +		return -ENOENT;
> +
> +	if (entry_key->keysz != table->tbl_keysz)
> +		return -EINVAL;
> +
> +	return p4tc_table_entry_create_bpf(pipeline, table, entry_key, act_bpf,
> +					   params->profile_id);

My understanding is this kfunc will allocate a "struct 
p4tc_table_entry_act_bpf_kern" object. If the bpf_p4tc_entry_delete() kfunc is 
never called and the bpf prog is unloaded, how the act_bpf object will be 
cleaned up?

> +}
> +
> +__bpf_kfunc static int
> +bpf_p4tc_entry_create(struct __sk_buff *skb_ctx,
> +		      struct p4tc_table_entry_create_bpf_params *params,
> +		      void *key, const u32 key__sz,
> +		      struct p4tc_table_entry_act_bpf *act_bpf)
> +{
> +	struct sk_buff *skb = (struct sk_buff *)skb_ctx;
> +	struct net *net;
> +
> +	net = skb->dev ? dev_net(skb->dev) : sock_net(skb->sk);
> +
> +	return __bpf_p4tc_entry_create(net, params, key, key__sz, act_bpf);
> +}
> +
> +__bpf_kfunc static int
> +xdp_p4tc_entry_create(struct xdp_md *xdp_ctx,
> +		      struct p4tc_table_entry_create_bpf_params *params,
> +		      void *key, const u32 key__sz,
> +		      struct p4tc_table_entry_act_bpf *act_bpf)
> +{
> +	struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
> +	struct net *net;
> +
> +	net = dev_net(ctx->rxq->dev);
> +
> +	return __bpf_p4tc_entry_create(net, params, key, key__sz, act_bpf);
> +}
> +
> +__bpf_kfunc static int
> +bpf_p4tc_entry_create_on_miss(struct __sk_buff *skb_ctx,
> +			      struct p4tc_table_entry_create_bpf_params *params,
> +			      void *key, const u32 key__sz,
> +			      struct p4tc_table_entry_act_bpf *act_bpf)
> +{
> +	struct sk_buff *skb = (struct sk_buff *)skb_ctx;
> +	struct net *net;
> +
> +	net = skb->dev ? dev_net(skb->dev) : sock_net(skb->sk);
> +
> +	return __bpf_p4tc_entry_create(net, params, key, key__sz, act_bpf);
> +}
> +
> +__bpf_kfunc static int
> +xdp_p4tc_entry_create_on_miss(struct xdp_md *xdp_ctx,

Same here. "struct xdp_buff *xdp".

> +			      struct p4tc_table_entry_create_bpf_params *params,
> +			      void *key, const u32 key__sz,
> +			      struct p4tc_table_entry_act_bpf *act_bpf)
> +{
> +	struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
> +	struct net *net;
> +
> +	net = dev_net(ctx->rxq->dev);
> +
> +	return __bpf_p4tc_entry_create(net, params, key, key__sz, act_bpf);
> +}
> +

[ ... ]

> +__bpf_kfunc static int
> +bpf_p4tc_entry_delete(struct __sk_buff *skb_ctx,
> +		      struct p4tc_table_entry_create_bpf_params *params,
> +		      void *key, const u32 key__sz)
> +{
> +	struct sk_buff *skb = (struct sk_buff *)skb_ctx;
> +	struct net *net;
> +
> +	net = skb->dev ? dev_net(skb->dev) : sock_net(skb->sk);
> +
> +	return __bpf_p4tc_entry_delete(net, params, key, key__sz);
> +}
> +
> +__bpf_kfunc static int
> +xdp_p4tc_entry_delete(struct xdp_md *xdp_ctx,
> +		      struct p4tc_table_entry_create_bpf_params *params,
> +		      void *key, const u32 key__sz)
> +{
> +	struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
> +	struct net *net;
> +
> +	net = dev_net(ctx->rxq->dev);
> +
> +	return __bpf_p4tc_entry_delete(net, params, key, key__sz);
> +}
> +
> +BTF_SET8_START(p4tc_kfunc_check_tbl_set_skb)

This soon will be broken with the latest change in bpf-next. It is replaced by 
BTF_KFUNCS_START. commit a05e90427ef6.

What is the plan on the selftest ?

> +BTF_ID_FLAGS(func, bpf_p4tc_tbl_read, KF_RET_NULL);
> +BTF_ID_FLAGS(func, bpf_p4tc_entry_create);
> +BTF_ID_FLAGS(func, bpf_p4tc_entry_create_on_miss);
> +BTF_ID_FLAGS(func, bpf_p4tc_entry_update);
> +BTF_ID_FLAGS(func, bpf_p4tc_entry_delete);
> +BTF_SET8_END(p4tc_kfunc_check_tbl_set_skb)
> +
> +static const struct btf_kfunc_id_set p4tc_kfunc_tbl_set_skb = {
> +	.owner = THIS_MODULE,
> +	.set = &p4tc_kfunc_check_tbl_set_skb,
> +};
> +
> +BTF_SET8_START(p4tc_kfunc_check_tbl_set_xdp)
> +BTF_ID_FLAGS(func, xdp_p4tc_tbl_read, KF_RET_NULL);
> +BTF_ID_FLAGS(func, xdp_p4tc_entry_create);
> +BTF_ID_FLAGS(func, xdp_p4tc_entry_create_on_miss);
> +BTF_ID_FLAGS(func, xdp_p4tc_entry_update);
> +BTF_ID_FLAGS(func, xdp_p4tc_entry_delete);
> +BTF_SET8_END(p4tc_kfunc_check_tbl_set_xdp)