Re: [PATCH net-next 2/8] netfilter: add API to manage NAT helpers.

[Flavio Leitner <fbl@sysclose.org>]

On Mon, Apr 01, 2019 at 12:10:32AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Mar 26, 2019 at 05:57:09PM -0300, Flavio Leitner wrote:
> > The API allows a conntrack helper to indicate its corresponding
> > NAT helper which then can be loaded and reference counted.
> > 
> > Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> > ---
> >  include/net/netfilter/nf_conntrack_helper.h |  19 +++-
> >  net/netfilter/nf_conntrack_amanda.c         |   2 +
> >  net/netfilter/nf_conntrack_ftp.c            |   6 +-
> >  net/netfilter/nf_conntrack_helper.c         | 108 +++++++++++++++++++-
> >  net/netfilter/nf_conntrack_irc.c            |   3 +-
> >  net/netfilter/nf_conntrack_sane.c           |   4 +-
> >  net/netfilter/nf_conntrack_sip.c            |  12 ++-
> >  net/netfilter/nf_conntrack_tftp.c           |   6 +-
> >  8 files changed, 147 insertions(+), 13 deletions(-)
> > 
> > diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
> > index e86fadf7e7c5..0d36d6bfb522 100644
> > --- a/include/net/netfilter/nf_conntrack_helper.h
> > +++ b/include/net/netfilter/nf_conntrack_helper.h
> > @@ -58,6 +58,8 @@ struct nf_conntrack_helper {
> >  	unsigned int queue_num;
> >  	/* length of userspace private data stored in nf_conn_help->data */
> >  	u16 data_len;
> > +	/* name of NAT helper module */
> > +	char nat_mod_name[NF_CT_HELPER_NAME_LEN];
> >  };
> >  
> >  /* Must be kept in sync with the classes defined by helpers */
> > @@ -98,7 +100,8 @@ void nf_ct_helper_init(struct nf_conntrack_helper *helper,
> >  				   enum ip_conntrack_info ctinfo),
> >  		       int (*from_nlattr)(struct nlattr *attr,
> >  					  struct nf_conn *ct),
> > -		       struct module *module);
> > +		       struct module *module,
> > +		       const char *nat_mod_name);
> >  
> >  int nf_conntrack_helper_register(struct nf_conntrack_helper *);
> >  void nf_conntrack_helper_unregister(struct nf_conntrack_helper *);
> > @@ -157,4 +160,18 @@ nf_ct_helper_expectfn_find_by_symbol(const void *symbol);
> >  extern struct hlist_head *nf_ct_helper_hash;
> >  extern unsigned int nf_ct_helper_hsize;
> >  
> > +struct nf_conntrack_helper_nat {
> > +	struct list_head list;
> > +	char name[NF_CT_HELPER_NAME_LEN];
> > +	struct module *module;		/* pointer to self */
> > +};
> > +
> > +void nf_ct_helper_nat_init(struct nf_conntrack_helper_nat *nat,
> > +			   const char *name, struct module *module);
> 
> Instead of this nf_ct_helper_nat_init() runtime initializer, define
> the structure in C99 as static in the NAT helper module?
> 
> Telling this because we can probably also extend this structure to
> remove the RCU hook between ct helper and nat helper at some point
> through this new definition.

Sounds good, let me try that.


> > +void nf_conntrack_helper_nat_register(struct nf_conntrack_helper_nat *nat);
> 
> Shorter name suggestion:
> 
>         nf_nat_helper_register()
> 
> > +void nf_conntrack_helper_nat_unregister(struct nf_conntrack_helper_nat *nat);
> 
>         nf_nat_helper_unregister()
> 
> > +int nf_conntrack_helper_nat_try_module_get(const char *name, u16 l3num,
> > +					   u8 protonum);
> 
>         nf_nat_helper_try_module_get()
> 
> > +void nf_conntrack_helper_nat_put(struct nf_conntrack_helper *helper);
> 
>         nf_nat_helper_nat_put()

Ok to all the above.

Thanks,
fbl