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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190415140404.GF3319@p50.lan>
Date:   Mon, 15 Apr 2019 11:04:04 -0300
From:   Flavio Leitner <fbl@...hat.com>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     netdev@...r.kernel.org, Joe Stringer <joe@....org>,
        Pravin B Shelar <pshelar@....org>, dev@...nvswitch.org,
        netfilter-devel@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/8] netfilter: add API to manage NAT helpers.

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);