[PATCH] netfilter: nf_sockopt_find() should return ERESTARTSYS

[PATCH] netfilter: nf_sockopt_find() should return ERESTARTSYS

[Eric Dumazet]

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 instead of -ERESTARTSYS if
a signal is pending.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/netfilter/nf_sockopt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_sockopt.c b/net/netfilter/nf_sockopt.c
index f042ae521557..37181447715b 100644
--- a/net/netfilter/nf_sockopt.c
+++ b/net/netfilter/nf_sockopt.c
@@ -66,7 +66,7 @@ static struct nf_sockopt_ops *nf_sockopt_find(struct sock *sk, u_int8_t pf,
 	struct nf_sockopt_ops *ops;
 
 	if (mutex_lock_interruptible(&nf_sockopt_mutex) != 0)
-		return ERR_PTR(-EINTR);
+		return ERR_PTR(-ERESTARTSYS);
 
 	list_for_each_entry(ops, &nf_sockopts, list) {
 		if (ops->pf == pf) {

Re: [PATCH] netfilter: nf_sockopt_find() should return ERESTARTSYS

[Patrick McHardy]

On 23. Juli 2014 23:53:15 GMT+01:00, Eric Dumazet <eric.dumazet@gmail.com> 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 instead of -ERESTARTSYS if
>a signal is pending.
>

I'd propose to simply use the non interruptable mutex functions. We
have many instances where this is really completely unnecessary.

I can take care of this (once my notebook has been repaired).

>Signed-off-by: Eric Dumazet <edumazet@google.com>
>---
> net/netfilter/nf_sockopt.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/netfilter/nf_sockopt.c b/net/netfilter/nf_sockopt.c
>index f042ae521557..37181447715b 100644
>--- a/net/netfilter/nf_sockopt.c
>+++ b/net/netfilter/nf_sockopt.c
>@@ -66,7 +66,7 @@ static struct nf_sockopt_ops *nf_sockopt_find(struct
>sock *sk, u_int8_t pf,
> 	struct nf_sockopt_ops *ops;
> 
> 	if (mutex_lock_interruptible(&nf_sockopt_mutex) != 0)
>-		return ERR_PTR(-EINTR);
>+		return ERR_PTR(-ERESTARTSYS);
> 
> 	list_for_each_entry(ops, &nf_sockopts, list) {
> 		if (ops->pf == pf) {
>
>

Re: [PATCH] netfilter: nf_sockopt_find() should return ERESTARTSYS

[Eric Dumazet]

On Thu, 2014-07-24 at 18:19 +0100, Patrick McHardy wrote:
> On 23. Juli 2014 23:53:15 GMT+01:00, Eric Dumazet <eric.dumazet@gmail.com> 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 instead of -ERESTARTSYS if
> >a signal is pending.
> >
> 
> I'd propose to simply use the non interruptable mutex functions. We
> have many instances where this is really completely unnecessary.
> 
> I can take care of this (once my notebook has been repaired).

No problem I'll send a v2, thanks.

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

[Eric Dumazet]

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.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
v2: at Patrick suggestion simply use mutex_lock()

 net/netfilter/nf_sockopt.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_sockopt.c b/net/netfilter/nf_sockopt.c
index f042ae521557..3e59a465407b 100644
--- a/net/netfilter/nf_sockopt.c
+++ b/net/netfilter/nf_sockopt.c
@@ -26,8 +26,7 @@ int nf_register_sockopt(struct nf_sockopt_ops *reg)
 	struct nf_sockopt_ops *ops;
 	int ret = 0;
 
-	if (mutex_lock_interruptible(&nf_sockopt_mutex) != 0)
-		return -EINTR;
+	mutex_lock(&nf_sockopt_mutex);
 
 	list_for_each_entry(ops, &nf_sockopts, list) {
 		if (ops->pf == reg->pf
@@ -65,8 +64,7 @@ static struct nf_sockopt_ops *nf_sockopt_find(struct sock *sk, u_int8_t pf,
 {
 	struct nf_sockopt_ops *ops;
 
-	if (mutex_lock_interruptible(&nf_sockopt_mutex) != 0)
-		return ERR_PTR(-EINTR);
+	mutex_lock(&nf_sockopt_mutex);
 
 	list_for_each_entry(ops, &nf_sockopts, list) {
 		if (ops->pf == pf) {

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

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

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.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 848 bytes --]

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;

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

[Patrick McHardy]

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.