* [PATCH] netfilter: nf_sockopt_find() should return ERESTARTSYS
@ 2014-07-23 22:53 Eric Dumazet
2014-07-24 17:19 ` Patrick McHardy
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2014-07-23 22:53 UTC (permalink / raw)
To: Pablo Neira Ayuso, David Miller; +Cc: netdev, netfilter-devel
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) {
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: nf_sockopt_find() should return ERESTARTSYS
2014-07-23 22:53 [PATCH] netfilter: nf_sockopt_find() should return ERESTARTSYS Eric Dumazet
@ 2014-07-24 17:19 ` Patrick McHardy
2014-07-24 20:35 ` Eric Dumazet
2014-07-24 20:41 ` [PATCH v2] netfilter: nf_sockopt_find() / nf_register_sockopt() should not return EINTR Eric Dumazet
0 siblings, 2 replies; 6+ messages in thread
From: Patrick McHardy @ 2014-07-24 17:19 UTC (permalink / raw)
To: Eric Dumazet, Pablo Neira Ayuso, David Miller; +Cc: netdev, netfilter-devel
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) {
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: nf_sockopt_find() should return ERESTARTSYS
2014-07-24 17:19 ` Patrick McHardy
@ 2014-07-24 20:35 ` Eric Dumazet
2014-07-24 20:41 ` [PATCH v2] netfilter: nf_sockopt_find() / nf_register_sockopt() should not return EINTR Eric Dumazet
1 sibling, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2014-07-24 20:35 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Pablo Neira Ayuso, David Miller, netdev, netfilter-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] netfilter: nf_sockopt_find() / nf_register_sockopt() should not return EINTR
2014-07-24 17:19 ` Patrick McHardy
2014-07-24 20:35 ` Eric Dumazet
@ 2014-07-24 20:41 ` Eric Dumazet
2014-07-31 10:55 ` Pablo Neira Ayuso
1 sibling, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2014-07-24 20:41 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Pablo Neira Ayuso, David Miller, netdev, netfilter-devel
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) {
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] netfilter: nf_sockopt_find() / nf_register_sockopt() should not return EINTR
2014-07-24 20:41 ` [PATCH v2] netfilter: nf_sockopt_find() / nf_register_sockopt() should not return EINTR Eric Dumazet
@ 2014-07-31 10:55 ` Pablo Neira Ayuso
2014-07-31 11:05 ` Patrick McHardy
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2014-07-31 10:55 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Patrick McHardy, David Miller, netdev, netfilter-devel
[-- 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;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] netfilter: nf_sockopt_find() / nf_register_sockopt() should not return EINTR
2014-07-31 10:55 ` Pablo Neira Ayuso
@ 2014-07-31 11:05 ` Patrick McHardy
0 siblings, 0 replies; 6+ messages in thread
From: Patrick McHardy @ 2014-07-31 11:05 UTC (permalink / raw)
To: Pablo Neira Ayuso, Eric Dumazet; +Cc: David Miller, netdev, netfilter-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-31 11:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-23 22:53 [PATCH] netfilter: nf_sockopt_find() should return ERESTARTSYS Eric Dumazet
2014-07-24 17:19 ` Patrick McHardy
2014-07-24 20:35 ` Eric Dumazet
2014-07-24 20:41 ` [PATCH v2] netfilter: nf_sockopt_find() / nf_register_sockopt() should not return EINTR Eric Dumazet
2014-07-31 10:55 ` Pablo Neira Ayuso
2014-07-31 11:05 ` Patrick McHardy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.