netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] several minor Netlink fixes
@ 2010-03-16 23:29 Pablo Neira Ayuso
  2010-03-16 23:29 ` [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err() Pablo Neira Ayuso
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Pablo Neira Ayuso @ 2010-03-16 23:29 UTC (permalink / raw)
  To: netdev; +Cc: kaber, davem

Hi!

This patchset contains a couple of fixes for Netlink related stuff. More
relevantly, a path that allows to set ENOBUFS when NETLINK_NO_ENOBUFS is
set (that should not happen). It follows the corresponding fix for
ctnetlink. This patchset closes with one fix for a unaligned access.

Please apply! Thanks!

---

Pablo Neira Ayuso (3):
      netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
      netfilter: ctnetlink: fix reliable event delivery if message building fails
      netlink: fix unaligned access in nla_get_be64()


 include/linux/netfilter/nfnetlink.h  |    2 +-
 include/linux/netlink.h              |    2 +-
 include/net/netlink.h                |    6 +++++-
 net/netfilter/nf_conntrack_netlink.c |    4 +++-
 net/netfilter/nfnetlink.c            |    4 ++--
 net/netlink/af_netlink.c             |   13 +++++++++++--
 6 files changed, 23 insertions(+), 8 deletions(-)


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
  2010-03-16 23:29 [PATCH 0/3] several minor Netlink fixes Pablo Neira Ayuso
@ 2010-03-16 23:29 ` Pablo Neira Ayuso
  2010-03-17  0:04   ` Pablo Neira Ayuso
  2010-03-17 15:26   ` Patrick McHardy
  2010-03-16 23:30 ` [PATCH 2/3] netfilter: ctnetlink: fix reliable event delivery if message building fails Pablo Neira Ayuso
  2010-03-16 23:30 ` [PATCH 3/3] netlink: fix unaligned access in nla_get_be64() Pablo Neira Ayuso
  2 siblings, 2 replies; 16+ messages in thread
From: Pablo Neira Ayuso @ 2010-03-16 23:29 UTC (permalink / raw)
  To: netdev; +Cc: kaber, davem

Currently, ENOBUFS errors are reported to the socket via
netlink_set_err() even if NETLINK_RECV_NO_ENOBUFS is set. However,
that should not happen. This fixes this problem and it changes the
prototype of netlink_set_err() to return the number of sockets whose
error has been set. This allows to know if any error has been set.
This return value is used in the next patch in these bugfix series.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netlink.h  |    2 +-
 net/netlink/af_netlink.c |   13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index fde27c0..6eaca5e 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -188,7 +188,7 @@ extern int netlink_has_listeners(struct sock *sk, unsigned int group);
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int nonblock);
 extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 pid,
 			     __u32 group, gfp_t allocation);
-extern void netlink_set_err(struct sock *ssk, __u32 pid, __u32 group, int code);
+extern int netlink_set_err(struct sock *ssk, __u32 pid, __u32 group, int code);
 extern int netlink_register_notifier(struct notifier_block *nb);
 extern int netlink_unregister_notifier(struct notifier_block *nb);
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 4c5972b..24ec840 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1104,8 +1104,12 @@ static inline int do_one_set_err(struct sock *sk,
 	    !test_bit(p->group - 1, nlk->groups))
 		goto out;
 
+	if (p->code == ENOBUFS && nlk->flags & NETLINK_RECV_NO_ENOBUFS) {
+		goto out;
+
 	sk->sk_err = p->code;
 	sk->sk_error_report(sk);
+	return 1;
 out:
 	return 0;
 }
@@ -1116,12 +1120,16 @@ out:
  * @pid: the PID of a process that we want to skip (if any)
  * @groups: the broadcast group that will notice the error
  * @code: error code, must be negative (as usual in kernelspace)
+ *
+ * This function returns the number of broadcast listeners whose error code
+ * has been set.
  */
-void netlink_set_err(struct sock *ssk, u32 pid, u32 group, int code)
+int netlink_set_err(struct sock *ssk, u32 pid, u32 group, int code)
 {
 	struct netlink_set_err_data info;
 	struct hlist_node *node;
 	struct sock *sk;
+	int ret = 0;
 
 	info.exclude_sk = ssk;
 	info.pid = pid;
@@ -1132,9 +1140,10 @@ void netlink_set_err(struct sock *ssk, u32 pid, u32 group, int code)
 	read_lock(&nl_table_lock);
 
 	sk_for_each_bound(sk, node, &nl_table[ssk->sk_protocol].mc_list)
-		do_one_set_err(sk, &info);
+		ret += do_one_set_err(sk, &info);
 
 	read_unlock(&nl_table_lock);
+	return ret;
 }
 EXPORT_SYMBOL(netlink_set_err);
 


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/3] netfilter: ctnetlink: fix reliable event delivery if message building fails
  2010-03-16 23:29 [PATCH 0/3] several minor Netlink fixes Pablo Neira Ayuso
  2010-03-16 23:29 ` [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err() Pablo Neira Ayuso
@ 2010-03-16 23:30 ` Pablo Neira Ayuso
  2010-03-16 23:30 ` [PATCH 3/3] netlink: fix unaligned access in nla_get_be64() Pablo Neira Ayuso
  2 siblings, 0 replies; 16+ messages in thread
From: Pablo Neira Ayuso @ 2010-03-16 23:30 UTC (permalink / raw)
  To: netdev; +Cc: kaber, davem

This patch fixes a bug that allows to lose events when reliable
event delivery mode is used, ie. if NETLINK_BROADCAST_SEND_ERROR
and NETLINK_RECV_NO_ENOBUFS socket options are set.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netfilter/nfnetlink.h  |    2 +-
 net/netfilter/nf_conntrack_netlink.c |    4 +++-
 net/netfilter/nfnetlink.c            |    4 ++--
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/linux/netfilter/nfnetlink.h b/include/linux/netfilter/nfnetlink.h
index 5392386..361d6b5 100644
--- a/include/linux/netfilter/nfnetlink.h
+++ b/include/linux/netfilter/nfnetlink.h
@@ -76,7 +76,7 @@ extern int nfnetlink_subsys_unregister(const struct nfnetlink_subsystem *n);
 extern int nfnetlink_has_listeners(struct net *net, unsigned int group);
 extern int nfnetlink_send(struct sk_buff *skb, struct net *net, u32 pid, unsigned group,
 			  int echo, gfp_t flags);
-extern void nfnetlink_set_err(struct net *net, u32 pid, u32 group, int error);
+extern int nfnetlink_set_err(struct net *net, u32 pid, u32 group, int error);
 extern int nfnetlink_unicast(struct sk_buff *skb, struct net *net, u_int32_t pid, int flags);
 
 extern void nfnl_lock(void);
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 8b05f36..00016e0 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -582,7 +582,9 @@ nla_put_failure:
 nlmsg_failure:
 	kfree_skb(skb);
 errout:
-	nfnetlink_set_err(net, 0, group, -ENOBUFS);
+	if (nfnetlink_set_err(net, 0, group, -ENOBUFS) > 0)
+		return -ENOBUFS;
+
 	return 0;
 }
 #endif /* CONFIG_NF_CONNTRACK_EVENTS */
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 8eb0cc2..6afa3d5 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -113,9 +113,9 @@ int nfnetlink_send(struct sk_buff *skb, struct net *net, u32 pid,
 }
 EXPORT_SYMBOL_GPL(nfnetlink_send);
 
-void nfnetlink_set_err(struct net *net, u32 pid, u32 group, int error)
+int nfnetlink_set_err(struct net *net, u32 pid, u32 group, int error)
 {
-	netlink_set_err(net->nfnl, pid, group, error);
+	return netlink_set_err(net->nfnl, pid, group, error);
 }
 EXPORT_SYMBOL_GPL(nfnetlink_set_err);
 


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/3] netlink: fix unaligned access in nla_get_be64()
  2010-03-16 23:29 [PATCH 0/3] several minor Netlink fixes Pablo Neira Ayuso
  2010-03-16 23:29 ` [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err() Pablo Neira Ayuso
  2010-03-16 23:30 ` [PATCH 2/3] netfilter: ctnetlink: fix reliable event delivery if message building fails Pablo Neira Ayuso
@ 2010-03-16 23:30 ` Pablo Neira Ayuso
  2010-03-20  5:44   ` David Miller
  2 siblings, 1 reply; 16+ messages in thread
From: Pablo Neira Ayuso @ 2010-03-16 23:30 UTC (permalink / raw)
  To: netdev; +Cc: kaber, davem

This patch fixes a unaligned access in nla_get_be64() that was
introduced by myself in a17c859849402315613a0015ac8fbf101acf0cc1.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netlink.h |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index a63b219..668ad04 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -945,7 +945,11 @@ static inline u64 nla_get_u64(const struct nlattr *nla)
  */
 static inline __be64 nla_get_be64(const struct nlattr *nla)
 {
-	return *(__be64 *) nla_data(nla);
+	__be64 tmp;
+
+	nla_memcpy(&tmp, nla, sizeof(tmp));
+
+	return tmp;
 }
 
 /**


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
  2010-03-16 23:29 ` [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err() Pablo Neira Ayuso
@ 2010-03-17  0:04   ` Pablo Neira Ayuso
  2010-03-17 15:26   ` Patrick McHardy
  1 sibling, 0 replies; 16+ messages in thread
From: Pablo Neira Ayuso @ 2010-03-17  0:04 UTC (permalink / raw)
  To: netdev; +Cc: kaber, davem

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

The previous patch was broken, sorry. Please, take this one instead. Thanks!

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

netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()

Currently, ENOBUFS errors are reported to the socket via
netlink_set_err() even if NETLINK_RECV_NO_ENOBUFS is set. However,
that should not happen. This fixes this problem and it changes the
prototype of netlink_set_err() to return the number of sockets whose
error has been set. This allows to know if any error has been set.
This return value is used in the next patch in these bugfix series.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netlink.h  |    2 +-
 net/netlink/af_netlink.c |   13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index fde27c0..6eaca5e 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -188,7 +188,7 @@ extern int netlink_has_listeners(struct sock *sk, unsigned int group);
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int nonblock);
 extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 pid,
 			     __u32 group, gfp_t allocation);
-extern void netlink_set_err(struct sock *ssk, __u32 pid, __u32 group, int code);
+extern int netlink_set_err(struct sock *ssk, __u32 pid, __u32 group, int code);
 extern int netlink_register_notifier(struct notifier_block *nb);
 extern int netlink_unregister_notifier(struct notifier_block *nb);
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 4c5972b..4ae2e8f 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1104,8 +1104,12 @@ static inline int do_one_set_err(struct sock *sk,
 	    !test_bit(p->group - 1, nlk->groups))
 		goto out;
 
+	if (p->code == ENOBUFS && nlk->flags & NETLINK_RECV_NO_ENOBUFS)
+		goto out;
+
 	sk->sk_err = p->code;
 	sk->sk_error_report(sk);
+	return 1;
 out:
 	return 0;
 }
@@ -1116,12 +1120,16 @@ out:
  * @pid: the PID of a process that we want to skip (if any)
  * @groups: the broadcast group that will notice the error
  * @code: error code, must be negative (as usual in kernelspace)
+ *
+ * This function returns the number of broadcast listeners whose error code
+ * has been set.
  */
-void netlink_set_err(struct sock *ssk, u32 pid, u32 group, int code)
+int netlink_set_err(struct sock *ssk, u32 pid, u32 group, int code)
 {
 	struct netlink_set_err_data info;
 	struct hlist_node *node;
 	struct sock *sk;
+	int ret = 0;
 
 	info.exclude_sk = ssk;
 	info.pid = pid;
@@ -1132,9 +1140,10 @@ void netlink_set_err(struct sock *ssk, u32 pid, u32 group, int code)
 	read_lock(&nl_table_lock);
 
 	sk_for_each_bound(sk, node, &nl_table[ssk->sk_protocol].mc_list)
-		do_one_set_err(sk, &info);
+		ret += do_one_set_err(sk, &info);
 
 	read_unlock(&nl_table_lock);
+	return ret;
 }
 EXPORT_SYMBOL(netlink_set_err);
 

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
  2010-03-16 23:29 ` [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err() Pablo Neira Ayuso
  2010-03-17  0:04   ` Pablo Neira Ayuso
@ 2010-03-17 15:26   ` Patrick McHardy
  2010-03-17 16:17     ` Pablo Neira Ayuso
  1 sibling, 1 reply; 16+ messages in thread
From: Patrick McHardy @ 2010-03-17 15:26 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netdev, davem

Pablo Neira Ayuso wrote:
> Currently, ENOBUFS errors are reported to the socket via
> netlink_set_err() even if NETLINK_RECV_NO_ENOBUFS is set. However,
> that should not happen. This fixes this problem and it changes the
> prototype of netlink_set_err() to return the number of sockets whose
> error has been set. This allows to know if any error has been set.
> This return value is used in the next patch in these bugfix series.

But that only happens if we have a message allocate error, which is
a different situation than rcvqueue overrun, which I thought the
original patch was supposed to handle (disable netlink congestion
control).

Is there any problem with these errors?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
  2010-03-17 15:26   ` Patrick McHardy
@ 2010-03-17 16:17     ` Pablo Neira Ayuso
  2010-03-18 13:02       ` Patrick McHardy
  0 siblings, 1 reply; 16+ messages in thread
From: Pablo Neira Ayuso @ 2010-03-17 16:17 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: netdev, davem

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> Currently, ENOBUFS errors are reported to the socket via
>> netlink_set_err() even if NETLINK_RECV_NO_ENOBUFS is set. However,
>> that should not happen. This fixes this problem and it changes the
>> prototype of netlink_set_err() to return the number of sockets whose
>> error has been set. This allows to know if any error has been set.
>> This return value is used in the next patch in these bugfix series.
> 
> But that only happens if we have a message allocate error, which is
> a different situation than rcvqueue overrun, which I thought the
> original patch was supposed to handle (disable netlink congestion
> control).

Yes, allocation is a different situation but we still report ENOBUFS to
user-space. I think that NETLINK_RECV_NO_ENOBUFS is there to a) disable
ENOBUFS reports to user-space and b) disable Netlink congestion.

> Is there any problem with these errors?

Specifically in ctnetlink, if we fail to allocate a message in ctnetlink
and NETLINK_RECV_NO_ENOBUFS is set, we still lose an event and that
should not happen.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
  2010-03-17 16:17     ` Pablo Neira Ayuso
@ 2010-03-18 13:02       ` Patrick McHardy
  2010-03-18 16:34         ` Pablo Neira Ayuso
  0 siblings, 1 reply; 16+ messages in thread
From: Patrick McHardy @ 2010-03-18 13:02 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netdev, davem

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>> Pablo Neira Ayuso wrote:
>>> Currently, ENOBUFS errors are reported to the socket via
>>> netlink_set_err() even if NETLINK_RECV_NO_ENOBUFS is set. However,
>>> that should not happen. This fixes this problem and it changes the
>>> prototype of netlink_set_err() to return the number of sockets whose
>>> error has been set. This allows to know if any error has been set.
>>> This return value is used in the next patch in these bugfix series.
>> But that only happens if we have a message allocate error, which is
>> a different situation than rcvqueue overrun, which I thought the
>> original patch was supposed to handle (disable netlink congestion
>> control).
> 
> Yes, allocation is a different situation but we still report ENOBUFS to
> user-space. I think that NETLINK_RECV_NO_ENOBUFS is there to a) disable
> ENOBUFS reports to user-space and b) disable Netlink congestion.
> 
>> Is there any problem with these errors?
> 
> Specifically in ctnetlink, if we fail to allocate a message in ctnetlink
> and NETLINK_RECV_NO_ENOBUFS is set, we still lose an event and that
> should not happen.

I assume you mean "not set"? Otherwise I fail to follow :)


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
  2010-03-18 13:02       ` Patrick McHardy
@ 2010-03-18 16:34         ` Pablo Neira Ayuso
  2010-03-18 16:46           ` Patrick McHardy
  0 siblings, 1 reply; 16+ messages in thread
From: Pablo Neira Ayuso @ 2010-03-18 16:34 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: netdev, davem

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> Patrick McHardy wrote:
>>> Pablo Neira Ayuso wrote:
>>>> Currently, ENOBUFS errors are reported to the socket via
>>>> netlink_set_err() even if NETLINK_RECV_NO_ENOBUFS is set. However,
>>>> that should not happen. This fixes this problem and it changes the
>>>> prototype of netlink_set_err() to return the number of sockets whose
>>>> error has been set. This allows to know if any error has been set.
>>>> This return value is used in the next patch in these bugfix series.
>>> But that only happens if we have a message allocate error, which is
>>> a different situation than rcvqueue overrun, which I thought the
>>> original patch was supposed to handle (disable netlink congestion
>>> control).
>> Yes, allocation is a different situation but we still report ENOBUFS to
>> user-space. I think that NETLINK_RECV_NO_ENOBUFS is there to a) disable
>> ENOBUFS reports to user-space and b) disable Netlink congestion.
>>
>>> Is there any problem with these errors?
>> Specifically in ctnetlink, if we fail to allocate a message in ctnetlink
>> and NETLINK_RECV_NO_ENOBUFS is set, we still lose an event and that
>> should not happen.
> 
> I assume you mean "not set"? Otherwise I fail to follow :)

OK, I'll try again :-)

Currently, no matter if NETLINK_RECV_NO_ENOBUFS is set or not: if we
fail to allocate the netlink message, then ctnetlink_conntrack_event()
returns 0. Thus, we report ENOBUFS to user-space and we lose the event.

With my patches, if NETLINK_RECV_NO_ENOBUFS is set and we fail to
allocate the message, we don't report ENOBUFS and we don't lose the event.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
  2010-03-18 16:34         ` Pablo Neira Ayuso
@ 2010-03-18 16:46           ` Patrick McHardy
  2010-03-18 17:01             ` Pablo Neira Ayuso
  0 siblings, 1 reply; 16+ messages in thread
From: Patrick McHardy @ 2010-03-18 16:46 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netdev, davem

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>> Pablo Neira Ayuso wrote:
>>> Yes, allocation is a different situation but we still report ENOBUFS to
>>> user-space. I think that NETLINK_RECV_NO_ENOBUFS is there to a) disable
>>> ENOBUFS reports to user-space and b) disable Netlink congestion.
>>>
>>>> Is there any problem with these errors?
>>> Specifically in ctnetlink, if we fail to allocate a message in ctnetlink
>>> and NETLINK_RECV_NO_ENOBUFS is set, we still lose an event and that
>>> should not happen.
>> I assume you mean "not set"? Otherwise I fail to follow :)
> 
> OK, I'll try again :-)
> 
> Currently, no matter if NETLINK_RECV_NO_ENOBUFS is set or not: if we
> fail to allocate the netlink message, then ctnetlink_conntrack_event()
> returns 0. Thus, we report ENOBUFS to user-space and we lose the event.
> 
> With my patches, if NETLINK_RECV_NO_ENOBUFS is set and we fail to
> allocate the message, we don't report ENOBUFS and we don't lose the event.

That last part is what keeps confusing me. With your patch, if the
ENOBUFS options is set, we don't report the error to userspace
and therefore don't return it to conntrack, thus we *do* loose the
event. Which is correct however.

Did I get it right this time? :)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
  2010-03-18 16:46           ` Patrick McHardy
@ 2010-03-18 17:01             ` Pablo Neira Ayuso
  2010-03-18 17:22               ` Patrick McHardy
  0 siblings, 1 reply; 16+ messages in thread
From: Pablo Neira Ayuso @ 2010-03-18 17:01 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: netdev, davem

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> Patrick McHardy wrote:
>>> Pablo Neira Ayuso wrote:
>>>> Yes, allocation is a different situation but we still report ENOBUFS to
>>>> user-space. I think that NETLINK_RECV_NO_ENOBUFS is there to a) disable
>>>> ENOBUFS reports to user-space and b) disable Netlink congestion.
>>>>
>>>>> Is there any problem with these errors?
>>>> Specifically in ctnetlink, if we fail to allocate a message in ctnetlink
>>>> and NETLINK_RECV_NO_ENOBUFS is set, we still lose an event and that
>>>> should not happen.
>>> I assume you mean "not set"? Otherwise I fail to follow :)
>> OK, I'll try again :-)
>>
>> Currently, no matter if NETLINK_RECV_NO_ENOBUFS is set or not: if we
>> fail to allocate the netlink message, then ctnetlink_conntrack_event()
>> returns 0. Thus, we report ENOBUFS to user-space and we lose the event.
>>
>> With my patches, if NETLINK_RECV_NO_ENOBUFS is set and we fail to
>> allocate the message, we don't report ENOBUFS and we don't lose the event.
> 
> That last part is what keeps confusing me. With your patch, if the
> ENOBUFS options is set, we don't report the error to userspace
> and therefore don't return it to conntrack, thus we *do* loose the
> event. Which is correct however.

Sorry, I'm being a bit imprecise myself: we do lose the event anyway.
However, with my patch, if the NO_ENOBUFS option is set, we keep the
event in the ctevent cache, so we can try to deliver it again with the
next packet (this is what I initially meant with "we don't lose the
event", yes, confusing...).

> Did I get it right this time? :)

I think so! :-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
  2010-03-18 17:01             ` Pablo Neira Ayuso
@ 2010-03-18 17:22               ` Patrick McHardy
  2010-03-19  0:24                 ` Pablo Neira Ayuso
  0 siblings, 1 reply; 16+ messages in thread
From: Patrick McHardy @ 2010-03-18 17:22 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netdev, davem

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>>> Currently, no matter if NETLINK_RECV_NO_ENOBUFS is set or not: if we
>>> fail to allocate the netlink message, then ctnetlink_conntrack_event()
>>> returns 0. Thus, we report ENOBUFS to user-space and we lose the event.
>>>
>>> With my patches, if NETLINK_RECV_NO_ENOBUFS is set and we fail to
>>> allocate the message, we don't report ENOBUFS and we don't lose the event.
>> That last part is what keeps confusing me. With your patch, if the
>> ENOBUFS options is set, we don't report the error to userspace
>> and therefore don't return it to conntrack, thus we *do* loose the
>> event. Which is correct however.
> 
> Sorry, I'm being a bit imprecise myself: we do lose the event anyway.
> However, with my patch, if the NO_ENOBUFS option is set, we keep the
> event in the ctevent cache, so we can try to deliver it again with the
> next packet (this is what I initially meant with "we don't lose the
> event", yes, confusing...).

That still doesn't make sense. The NO_ENOBUFS option *surpresses*
errors, so conntrack assumes success and we *don't* keep it in the
cache. Look:

Patch 1:

> @@ -1104,8 +1104,12 @@ static inline int do_one_set_err(struct sock *sk,
>  	    !test_bit(p->group - 1, nlk->groups))
>  		goto out;
>  
> +	if (p->code == ENOBUFS && nlk->flags & NETLINK_RECV_NO_ENOBUFS)
> +		goto out;
> +
>  	sk->sk_err = p->code;
>  	sk->sk_error_report(sk);
> +	return 1;
>  out:
>  	return 0;
>  }

=> return 0 for NO_ENOBUFS option

Patch 2:

> +	if (nfnetlink_set_err(net, 0, group, -ENOBUFS) > 0)
> +		return -ENOBUFS;
> +
>  	return 0;
>  }

=> return 0 to conntrack. Therefore nf_conntrack_eventmask_report()
assumes success. So if the NO_ENOBUFS option is indeed used for
reliable delivery, this won't work.

Generally the logic seems inverted, you should return an error
to conntrack if userspace wasn't notified of the error.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
  2010-03-18 17:22               ` Patrick McHardy
@ 2010-03-19  0:24                 ` Pablo Neira Ayuso
  2010-03-20 21:30                   ` David Miller
  0 siblings, 1 reply; 16+ messages in thread
From: Pablo Neira Ayuso @ 2010-03-19  0:24 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: netdev, davem

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

Patrick McHardy wrote:
> Generally the logic seems inverted, you should return an error
> to conntrack if userspace wasn't notified of the error.

Indeed, thanks. Are you OK with this patch instead?


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

netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()

Currently, ENOBUFS errors are reported to the socket via
netlink_set_err() even if NETLINK_RECV_NO_ENOBUFS is set. However,
that should not happen. This fixes this problem and it changes the
prototype of netlink_set_err() to return the number of sockets that
have set the NETLINK_RECV_NO_ENOBUFS socket option. This return
value is used in the next patch in these bugfix series.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netlink.h  |    2 +-
 net/netlink/af_netlink.c |   17 ++++++++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index fde27c0..6eaca5e 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -188,7 +188,7 @@ extern int netlink_has_listeners(struct sock *sk, unsigned int group);
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int nonblock);
 extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 pid,
 			     __u32 group, gfp_t allocation);
-extern void netlink_set_err(struct sock *ssk, __u32 pid, __u32 group, int code);
+extern int netlink_set_err(struct sock *ssk, __u32 pid, __u32 group, int code);
 extern int netlink_register_notifier(struct notifier_block *nb);
 extern int netlink_unregister_notifier(struct notifier_block *nb);
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 4c5972b..0052d3c 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1093,6 +1093,7 @@ static inline int do_one_set_err(struct sock *sk,
 				 struct netlink_set_err_data *p)
 {
 	struct netlink_sock *nlk = nlk_sk(sk);
+	int ret = 0;
 
 	if (sk == p->exclude_sk)
 		goto out;
@@ -1104,10 +1105,15 @@ static inline int do_one_set_err(struct sock *sk,
 	    !test_bit(p->group - 1, nlk->groups))
 		goto out;
 
+	if (p->code == ENOBUFS && nlk->flags & NETLINK_RECV_NO_ENOBUFS) {
+		ret = 1;
+		goto out;
+	}
+
 	sk->sk_err = p->code;
 	sk->sk_error_report(sk);
 out:
-	return 0;
+	return ret;
 }
 
 /**
@@ -1116,12 +1122,16 @@ out:
  * @pid: the PID of a process that we want to skip (if any)
  * @groups: the broadcast group that will notice the error
  * @code: error code, must be negative (as usual in kernelspace)
+ *
+ * This function returns the number of broadcast listeners that have set the
+ * NETLINK_RECV_NO_ENOBUFS socket option.
  */
-void netlink_set_err(struct sock *ssk, u32 pid, u32 group, int code)
+int netlink_set_err(struct sock *ssk, u32 pid, u32 group, int code)
 {
 	struct netlink_set_err_data info;
 	struct hlist_node *node;
 	struct sock *sk;
+	int ret = 0;
 
 	info.exclude_sk = ssk;
 	info.pid = pid;
@@ -1132,9 +1142,10 @@ void netlink_set_err(struct sock *ssk, u32 pid, u32 group, int code)
 	read_lock(&nl_table_lock);
 
 	sk_for_each_bound(sk, node, &nl_table[ssk->sk_protocol].mc_list)
-		do_one_set_err(sk, &info);
+		ret += do_one_set_err(sk, &info);
 
 	read_unlock(&nl_table_lock);
+	return ret;
 }
 EXPORT_SYMBOL(netlink_set_err);
 

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] netlink: fix unaligned access in nla_get_be64()
  2010-03-16 23:30 ` [PATCH 3/3] netlink: fix unaligned access in nla_get_be64() Pablo Neira Ayuso
@ 2010-03-20  5:44   ` David Miller
  0 siblings, 0 replies; 16+ messages in thread
From: David Miller @ 2010-03-20  5:44 UTC (permalink / raw)
  To: pablo; +Cc: netdev, kaber

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Wed, 17 Mar 2010 00:30:44 +0100

> This patch fixes a unaligned access in nla_get_be64() that was
> introduced by myself in a17c859849402315613a0015ac8fbf101acf0cc1.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Applied.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
  2010-03-19  0:24                 ` Pablo Neira Ayuso
@ 2010-03-20 21:30                   ` David Miller
  2010-03-22 15:38                     ` Patrick McHardy
  0 siblings, 1 reply; 16+ messages in thread
From: David Miller @ 2010-03-20 21:30 UTC (permalink / raw)
  To: pablo; +Cc: kaber, netdev

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Fri, 19 Mar 2010 01:24:42 +0100

> Patrick McHardy wrote:
>> Generally the logic seems inverted, you should return an error
>> to conntrack if userspace wasn't notified of the error.
> 
> Indeed, thanks. Are you OK with this patch instead?

I went over all of this and now the patches #1 and #2 look
correct to me, so I've applied them to net-2.6

Patrick let me know if you think any follow-on tidy ups
are still necessary and we can add them.

Thanks Pablo!

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
  2010-03-20 21:30                   ` David Miller
@ 2010-03-22 15:38                     ` Patrick McHardy
  0 siblings, 0 replies; 16+ messages in thread
From: Patrick McHardy @ 2010-03-22 15:38 UTC (permalink / raw)
  To: David Miller; +Cc: pablo, netdev

David Miller wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Fri, 19 Mar 2010 01:24:42 +0100
>
>   
>> Patrick McHardy wrote:
>>     
>>> Generally the logic seems inverted, you should return an error
>>> to conntrack if userspace wasn't notified of the error.
>>>       
>> Indeed, thanks. Are you OK with this patch instead?
>>     
>
> I went over all of this and now the patches #1 and #2 look
> correct to me, so I've applied them to net-2.6
>
> Patrick let me know if you think any follow-on tidy ups
> are still necessary and we can add them.
The patch looks fine to me as well, thanks Dave.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2010-03-22 15:38 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-16 23:29 [PATCH 0/3] several minor Netlink fixes Pablo Neira Ayuso
2010-03-16 23:29 ` [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err() Pablo Neira Ayuso
2010-03-17  0:04   ` Pablo Neira Ayuso
2010-03-17 15:26   ` Patrick McHardy
2010-03-17 16:17     ` Pablo Neira Ayuso
2010-03-18 13:02       ` Patrick McHardy
2010-03-18 16:34         ` Pablo Neira Ayuso
2010-03-18 16:46           ` Patrick McHardy
2010-03-18 17:01             ` Pablo Neira Ayuso
2010-03-18 17:22               ` Patrick McHardy
2010-03-19  0:24                 ` Pablo Neira Ayuso
2010-03-20 21:30                   ` David Miller
2010-03-22 15:38                     ` Patrick McHardy
2010-03-16 23:30 ` [PATCH 2/3] netfilter: ctnetlink: fix reliable event delivery if message building fails Pablo Neira Ayuso
2010-03-16 23:30 ` [PATCH 3/3] netlink: fix unaligned access in nla_get_be64() Pablo Neira Ayuso
2010-03-20  5:44   ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).