Re: [PATCH] netfilter: replace -EEXIST with -EBUSY

Daniel Gomez <[email protected]> wrote:
> From: Daniel Gomez <[email protected]>
> 
> The -EEXIST error code is reserved by the module loading infrastructure
> to indicate that a module is already loaded. When a module's init
> function returns -EEXIST, userspace tools like kmod interpret this as
> "module already loaded" and treat the operation as successful, returning
> 0 to the user even though the module initialization actually failed.
>
> This follows the precedent set by commit 54416fd76770 ("netfilter:
> conntrack: helper: Replace -EEXIST by -EBUSY") which fixed the same
> issue in nf_conntrack_helper_register().
> 
> Affected modules:
>   * ebtable_broute ebtable_filter ebtable_nat arptable_filter
>   * ip6table_filter ip6table_mangle ip6table_nat ip6table_raw
>   * ip6table_security iptable_filter iptable_mangle iptable_nat
>   * iptable_raw iptable_security

But this is very different from what 54416fd76770 fixes.

Before 54416fd76770. userspace can make a configuration entry that
prevents and unrelated module from getting loaded but at the same time
it doesn't provide any error to userspace.

All these -EEXIST should not be possible unless the module is
already loaded.

> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 5697e3949a36..a04fc1757528 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -1299,7 +1299,7 @@ int ebt_register_template(const struct ebt_table *t, int (*table_init)(struct ne
>  	list_for_each_entry(tmpl, &template_tables, list) {
>  		if (WARN_ON_ONCE(strcmp(t->name, tmpl->name) == 0)) {
>  			mutex_unlock(&ebt_mutex);
> -			return -EEXIST;
> +			return -EBUSY;

As you can see from the WARN_ON, this cannot happen unless someone adds a new ebt kernel
table module that tries to register the same name.

> diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
> index 74cef8bf554c..62cf6a30875e 100644
> --- a/net/netfilter/nf_log.c
> +++ b/net/netfilter/nf_log.c
> @@ -89,7 +89,7 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  	if (pf == NFPROTO_UNSPEC) {
>  		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
>  			if (rcu_access_pointer(loggers[i][logger->type])) {
> -				ret = -EEXIST;
> +				ret = -EBUSY;
>  				goto unlock;

I don't see how this can happen, unless someone adds a new kernel module
that claims the same type as an existing kernel module.

> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index 90b7630421c4..48105ea3df15 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -1764,7 +1764,7 @@ EXPORT_SYMBOL_GPL(xt_hook_ops_alloc);
>  int xt_register_template(const struct xt_table *table,
>  			 int (*table_init)(struct net *net))
>  {
> -	int ret = -EEXIST, af = table->af;
> +	int ret = -EBUSY, af = table->af;
>  	struct xt_template *t;

Same, this requires someone adding a new kernel module with clashing
name.

I'll apply this patch but its not related to 54416fd76770 afaics.