Re: [PATCH bpf-next] net: netfilter: move bpf_ct_set_nat_info kfunc in nf_nat_bpf.c

[Yauheni Kaliuta <ykaliuta@redhat.com>]

Hi, Lorenzo!

Tested-by: Yauheni Kaliuta <ykaliuta@redhat.com>

>>>>> On Sun, 25 Sep 2022 15:26:12 +0200, Lorenzo Bianconi  wrote:

 > Remove circular dependency between nf_nat module and nf_conntrack one
 > moving bpf_ct_set_nat_info kfunc in nf_nat_bpf.c

 > Fixes: 0fabd2aa199f ("net: netfilter: add bpf_ct_set_nat_info kfunc helper")
 > Suggested-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
 > Tested-by: Nathan Chancellor <nathan@kernel.org>
 > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
 > ---
 >  include/net/netfilter/nf_conntrack_bpf.h |  5 ++
 >  include/net/netfilter/nf_nat.h           | 14 +++++
 >  net/netfilter/Makefile                   |  6 ++
 >  net/netfilter/nf_conntrack_bpf.c         | 49 ---------------
 >  net/netfilter/nf_nat_bpf.c               | 79 ++++++++++++++++++++++++
 >  net/netfilter/nf_nat_core.c              |  2 +-
 >  6 files changed, 105 insertions(+), 50 deletions(-)
 >  create mode 100644 net/netfilter/nf_nat_bpf.c

 > diff --git a/include/net/netfilter/nf_conntrack_bpf.h b/include/net/netfilter/nf_conntrack_bpf.h
 > index c8b80add1142..1ce46e406062 100644
 > --- a/include/net/netfilter/nf_conntrack_bpf.h
 > +++ b/include/net/netfilter/nf_conntrack_bpf.h
 > @@ -4,6 +4,11 @@
 >  #define _NF_CONNTRACK_BPF_H
 
 >  #include <linux/kconfig.h>
 > +#include <net/netfilter/nf_conntrack.h>
 > +
 > +struct nf_conn___init {
 > +	struct nf_conn ct;
 > +};
 
 >  #if (IS_BUILTIN(CONFIG_NF_CONNTRACK) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) || \
 >      (IS_MODULE(CONFIG_NF_CONNTRACK) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
 > diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h
 > index e9eb01e99d2f..cd084059a953 100644
 > --- a/include/net/netfilter/nf_nat.h
 > +++ b/include/net/netfilter/nf_nat.h
 > @@ -68,6 +68,20 @@ static inline bool nf_nat_oif_changed(unsigned int hooknum,
 >  #endif
 >  }
 
 > +#if (IS_BUILTIN(CONFIG_NF_NAT) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) || \
 > +    (IS_MODULE(CONFIG_NF_NAT) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
 > +
 > +extern int register_nf_nat_bpf(void);
 > +
 > +#else
 > +
 > +static inline int register_nf_nat_bpf(void)
 > +{
 > +	return 0;
 > +}
 > +
 > +#endif
 > +
 >  int nf_nat_register_fn(struct net *net, u8 pf, const struct nf_hook_ops *ops,
 >  		       const struct nf_hook_ops *nat_ops, unsigned int ops_count);
 >  void nf_nat_unregister_fn(struct net *net, u8 pf, const struct nf_hook_ops *ops,
 > diff --git a/net/netfilter/Makefile b/net/netfilter/Makefile
 > index 06df49ea6329..0f060d100880 100644
 > --- a/net/netfilter/Makefile
 > +++ b/net/netfilter/Makefile
 > @@ -60,6 +60,12 @@ obj-$(CONFIG_NF_NAT) += nf_nat.o
 >  nf_nat-$(CONFIG_NF_NAT_REDIRECT) += nf_nat_redirect.o
 >  nf_nat-$(CONFIG_NF_NAT_MASQUERADE) += nf_nat_masquerade.o
 
 > +ifeq ($(CONFIG_NF_NAT),m)
 > +nf_nat-$(CONFIG_DEBUG_INFO_BTF_MODULES) += nf_nat_bpf.o
 > +else ifeq ($(CONFIG_NF_NAT),y)
 > +nf_nat-$(CONFIG_DEBUG_INFO_BTF) += nf_nat_bpf.o
 > +endif
 > +
 >  # NAT helpers
 >  obj-$(CONFIG_NF_NAT_AMANDA) += nf_nat_amanda.o
 >  obj-$(CONFIG_NF_NAT_FTP) += nf_nat_ftp.o
 > diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
 > index 756ea818574e..f4ba4ff3a63b 100644
 > --- a/net/netfilter/nf_conntrack_bpf.c
 > +++ b/net/netfilter/nf_conntrack_bpf.c
 > @@ -14,7 +14,6 @@
 >  #include <linux/types.h>
 >  #include <linux/btf_ids.h>
 >  #include <linux/net_namespace.h>
 > -#include <net/netfilter/nf_conntrack.h>
 >  #include <net/netfilter/nf_conntrack_bpf.h>
 >  #include <net/netfilter/nf_conntrack_core.h>
 >  #include <net/netfilter/nf_nat.h>
 > @@ -239,10 +238,6 @@ __diag_push();
 >  __diag_ignore_all("-Wmissing-prototypes",
 >  		  "Global functions as their definitions will be in nf_conntrack BTF");
 
 > -struct nf_conn___init {
 > -	struct nf_conn ct;
 > -};
 > -
 >  /* bpf_xdp_ct_alloc - Allocate a new CT entry
 >   *
 >   * Parameters:
 > @@ -476,49 +471,6 @@ int bpf_ct_change_status(struct nf_conn *nfct, u32 status)
 >  	return nf_ct_change_status_common(nfct, status);
 >  }
 
 > -/* bpf_ct_set_nat_info - Set source or destination nat address
 > - *
 > - * Set source or destination nat address of the newly allocated
 > - * nf_conn before insertion. This must be invoked for referenced
 > - * PTR_TO_BTF_ID to nf_conn___init.
 > - *
 > - * Parameters:
 > - * @nfct	- Pointer to referenced nf_conn object, obtained using
 > - *		  bpf_xdp_ct_alloc or bpf_skb_ct_alloc.
 > - * @addr	- Nat source/destination address
 > - * @port	- Nat source/destination port. Non-positive values are
 > - *		  interpreted as select a random port.
 > - * @manip	- NF_NAT_MANIP_SRC or NF_NAT_MANIP_DST
 > - */
 > -int bpf_ct_set_nat_info(struct nf_conn___init *nfct,
 > -			union nf_inet_addr *addr, int port,
 > -			enum nf_nat_manip_type manip)
 > -{
 > -#if ((IS_MODULE(CONFIG_NF_NAT) && IS_MODULE(CONFIG_NF_CONNTRACK)) || \
 > -     IS_BUILTIN(CONFIG_NF_NAT))
 > -	struct nf_conn *ct = (struct nf_conn *)nfct;
 > -	u16 proto = nf_ct_l3num(ct);
 > -	struct nf_nat_range2 range;
 > -
 > -	if (proto != NFPROTO_IPV4 && proto != NFPROTO_IPV6)
 > -		return -EINVAL;
 > -
 > -	memset(&range, 0, sizeof(struct nf_nat_range2));
 > -	range.flags = NF_NAT_RANGE_MAP_IPS;
 > -	range.min_addr = *addr;
 > -	range.max_addr = range.min_addr;
 > -	if (port > 0) {
 > -		range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
 > -		range.min_proto.all = cpu_to_be16(port);
 > -		range.max_proto.all = range.min_proto.all;
 > -	}
 > -
 > -	return nf_nat_setup_info(ct, &range, manip) == NF_DROP ? -ENOMEM : 0;
 > -#else
 > -	return -EOPNOTSUPP;
 > -#endif
 > -}
 > -
 >  __diag_pop()
 
 >  BTF_SET8_START(nf_ct_kfunc_set)
 > @@ -532,7 +484,6 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
 >  BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
 >  BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
 >  BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
 > -BTF_ID_FLAGS(func, bpf_ct_set_nat_info, KF_TRUSTED_ARGS)
 >  BTF_SET8_END(nf_ct_kfunc_set)
 
 >  static const struct btf_kfunc_id_set nf_conntrack_kfunc_set = {
 > diff --git a/net/netfilter/nf_nat_bpf.c b/net/netfilter/nf_nat_bpf.c
 > new file mode 100644
 > index 000000000000..0fa5a0bbb0ff
 > --- /dev/null
 > +++ b/net/netfilter/nf_nat_bpf.c
 > @@ -0,0 +1,79 @@
 > +// SPDX-License-Identifier: GPL-2.0-only
 > +/* Unstable NAT Helpers for XDP and TC-BPF hook
 > + *
 > + * These are called from the XDP and SCHED_CLS BPF programs. Note that it is
 > + * allowed to break compatibility for these functions since the interface they
 > + * are exposed through to BPF programs is explicitly unstable.
 > + */
 > +
 > +#include <linux/bpf.h>
 > +#include <linux/btf_ids.h>
 > +#include <net/netfilter/nf_conntrack_bpf.h>
 > +#include <net/netfilter/nf_conntrack_core.h>
 > +#include <net/netfilter/nf_nat.h>
 > +
 > +__diag_push();
 > +__diag_ignore_all("-Wmissing-prototypes",
 > +		  "Global functions as their definitions will be in nf_nat BTF");
 > +
 > +/* bpf_ct_set_nat_info - Set source or destination nat address
 > + *
 > + * Set source or destination nat address of the newly allocated
 > + * nf_conn before insertion. This must be invoked for referenced
 > + * PTR_TO_BTF_ID to nf_conn___init.
 > + *
 > + * Parameters:
 > + * @nfct	- Pointer to referenced nf_conn object, obtained using
 > + *		  bpf_xdp_ct_alloc or bpf_skb_ct_alloc.
 > + * @addr	- Nat source/destination address
 > + * @port	- Nat source/destination port. Non-positive values are
 > + *		  interpreted as select a random port.
 > + * @manip	- NF_NAT_MANIP_SRC or NF_NAT_MANIP_DST
 > + */
 > +int bpf_ct_set_nat_info(struct nf_conn___init *nfct,
 > +			union nf_inet_addr *addr, int port,
 > +			enum nf_nat_manip_type manip)
 > +{
 > +	struct nf_conn *ct = (struct nf_conn *)nfct;
 > +	u16 proto = nf_ct_l3num(ct);
 > +	struct nf_nat_range2 range;
 > +
 > +	if (proto != NFPROTO_IPV4 && proto != NFPROTO_IPV6)
 > +		return -EINVAL;
 > +
 > +	memset(&range, 0, sizeof(struct nf_nat_range2));
 > +	range.flags = NF_NAT_RANGE_MAP_IPS;
 > +	range.min_addr = *addr;
 > +	range.max_addr = range.min_addr;
 > +	if (port > 0) {
 > +		range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
 > +		range.min_proto.all = cpu_to_be16(port);
 > +		range.max_proto.all = range.min_proto.all;
 > +	}
 > +
 > +	return nf_nat_setup_info(ct, &range, manip) == NF_DROP ? -ENOMEM : 0;
 > +}
 > +
 > +__diag_pop()
 > +
 > +BTF_SET8_START(nf_nat_kfunc_set)
 > +BTF_ID_FLAGS(func, bpf_ct_set_nat_info, KF_TRUSTED_ARGS)
 > +BTF_SET8_END(nf_nat_kfunc_set)
 > +
 > +static const struct btf_kfunc_id_set nf_bpf_nat_kfunc_set = {
 > +	.owner = THIS_MODULE,
 > +	.set   = &nf_nat_kfunc_set,
 > +};
 > +
 > +int register_nf_nat_bpf(void)
 > +{
 > +	int ret;
 > +
 > +	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP,
 > +					&nf_bpf_nat_kfunc_set);
 > +	if (ret)
 > +		return ret;
 > +
 > +	return register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS,
 > +					 &nf_bpf_nat_kfunc_set);
 > +}
 > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
 > index 7981be526f26..1ed09c9af5e5 100644
 > --- a/net/netfilter/nf_nat_core.c
 > +++ b/net/netfilter/nf_nat_core.c
 > @@ -1152,7 +1152,7 @@ static int __init nf_nat_init(void)
 >  	WARN_ON(nf_nat_hook != NULL);
 >  	RCU_INIT_POINTER(nf_nat_hook, &nat_hook);
 
 > -	return 0;
 > +	return register_nf_nat_bpf();
 >  }
 
 >  static void __exit nf_nat_cleanup(void)
 > -- 

 > 2.37.3


-- 
WBR,
Yauheni Kaliuta