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

[Flavio Leitner <fbl@redhat.com>]

On Mon, Apr 15, 2019 at 07:48:20AM +0200, Pablo Neira Ayuso wrote:
> Sorry I didn't see this in the first review.
> 
> On Sat, Apr 13, 2019 at 08:17:10PM -0300, Flavio Leitner wrote:
> [...]
> > +int
> > +nf_nat_helper_try_module_get(const char *name, u16 l3num, u8 protonum)
> > +{
> > +	struct nf_conntrack_helper *h;
> > +	struct nf_conntrack_nat_helper *nat;
> > +	char mod_name[NF_CT_HELPER_NAME_LEN];
> > +	int ret = 0;
> > +
> > +	rcu_read_lock();
> > +	h = __nf_conntrack_helper_find(name, l3num, protonum);
> > +	if (h == NULL) {
> > +		rcu_read_unlock();
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!strlen(h->nat_mod_name)) {
> > +		rcu_read_unlock();
> > +		return -EOPNOTSUPP;
> 
> Probably check for this at registration?

I thought to have a list of all helpers in use and then use
nat_mod_name to indicate if a NAT helper is supported or not.
I will change that to list only ones with NAT helper available
as I don't have a strong preference.

> > +	}
> > +
> > +	nat = nf_conntrack_nat_helper_find(h->nat_mod_name);
> > +	if (nat == NULL) {
> > +		snprintf(mod_name, sizeof(mod_name), "%s", h->nat_mod_name);
> > +		rcu_read_unlock();
> > +		ret = request_module(mod_name);
> > +		if (ret != 0)
> > +			return ret;
> 
> Not sure it is worth checking for request_module() return value, the
> code just below already is doing this.

Well, the above returns a more accurate error which helps
debugging/tracing problems. But since you said that, I checked
other cases calling request_module() and I am going to fix the
above.

> > +
> > +		rcu_read_lock();
> > +		nat = nf_conntrack_nat_helper_find(mod_name);
> > +		if (nat == NULL) {
> > +			rcu_read_unlock();
> > +			return -EINVAL;
> 
> ENOENT?

Makes sense.


> > +		}
> > +	}
> > +
> > +	if (!try_module_get(nat->module))
> > +		ret = -EINVAL;
> 
> ENOENT?
>
> Telling this because we will at some point propagate this error value
> to userspace by when we start using this infrastructure you're working
> on. EINVAL is already very overload in netlink and we'll use it from
> there.

Sounds good, thanks for the review!
fbl


> 
> > +
> > +	rcu_read_unlock();
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(nf_nat_helper_try_module_get);