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