diff mbox

[v2] netfilter: nf_sockopt_find() / nf_register_sockopt() should not return EINTR

Message ID 20140731105525.GA4680@salvia
State RFC
Delegated to: Pablo Neira
Headers show

Commit Message

Pablo Neira Ayuso July 31, 2014, 10:55 a.m. UTC 
On Thu, Jul 24, 2014 at 10:41:35PM +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> getsockopt() or setsockopt() sometimes returns -EINTR instead of
> -ENOPROTOOPT, causing headaches to application developers.
> 
> This is because unsupported commands might go through nf_sockopt_find()
> and this function returns -EINTR if a signal is pending.
> 
> Just use non interruptible mutex functions, as there is no reason
> we should sleep for a long time here.

On top of this patch, I think that at least we can also ged rid of
these interruptible mutex from the netfilter/core code too (see
preliminary patch attached).

I can also adapt the callers so they don't check anymore the return
value as they will always succeed.

Comments? Thanks.

Comments

Patrick McHardy July 31, 2014, 11:05 a.m. UTC | #1 
On 31. Juli 2014 11:55:25 GMT+01:00, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>On Thu, Jul 24, 2014 at 10:41:35PM +0200, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>> 
>> getsockopt() or setsockopt() sometimes returns -EINTR instead of
>> -ENOPROTOOPT, causing headaches to application developers.
>> 
>> This is because unsupported commands might go through
>nf_sockopt_find()
>> and this function returns -EINTR if a signal is pending.
>> 
>> Just use non interruptible mutex functions, as there is no reason
>> we should sleep for a long time here.
>
>On top of this patch, I think that at least we can also ged rid of
>these interruptible mutex from the netfilter/core code too (see
>preliminary patch attached).

Agreed. I think there are actually no cases at all where using the
interruptable variants makes sense.

>I can also adapt the callers so they don't check anymore the return
>value as they will always succeed.
>
>Comments? Thanks.

I'd leave checks in the callers at least for all init/registration functions.
Quite possible future changes will add back error conditions and its
more consistent to follow this convention in all cases.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 1fbab0c..a93c97f 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -35,11 +35,7 @@  EXPORT_SYMBOL_GPL(nf_ipv6_ops);
 
 int nf_register_afinfo(const struct nf_afinfo *afinfo)
 {
-	int err;
-
-	err = mutex_lock_interruptible(&afinfo_mutex);
-	if (err < 0)
-		return err;
+	mutex_lock(&afinfo_mutex);
 	RCU_INIT_POINTER(nf_afinfo[afinfo->family], afinfo);
 	mutex_unlock(&afinfo_mutex);
 	return 0;
@@ -68,11 +64,8 @@  static DEFINE_MUTEX(nf_hook_mutex);
 int nf_register_hook(struct nf_hook_ops *reg)
 {
 	struct nf_hook_ops *elem;
-	int err;
 
-	err = mutex_lock_interruptible(&nf_hook_mutex);
-	if (err < 0)
-		return err;
+	mutex_lock(&nf_hook_mutex);
 	list_for_each_entry(elem, &nf_hooks[reg->pf][reg->hooknum], list) {
 		if (reg->priority < elem->priority)
 			break;