[PATCH] netlink: add NETLINK_BROADCAST_ERROR socket option

[PATCH] netlink: add NETLINK_BROADCAST_ERROR socket option

[Pablo Neira Ayuso]

This patch adds NETLINK_BROADCAST_ERROR which is a netlink
socket option that the listener can set to make netlink_broadcast()
return errors in the delivery to the caller. This option is useful
if the caller of netlink_broadcast() do something with the result
of the message delivery, like in ctnetlink where it drops a network
packet if the event delivery failed, this is used to enable reliable
logging and state-synchronization. If this socket option is not set,
netlink_broadcast() only reports ESRCH errors and silently ignore
ENOBUFS errors, which is what most netlink_broadcast() callers
should do.

This socket option is based on a suggestion from Patrick McHardy.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---

 include/linux/netlink.h  |    1 +
 net/netlink/af_netlink.c |   26 +++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 51b09a1..1e6bf99 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -103,6 +103,7 @@ struct nlmsgerr
 #define NETLINK_ADD_MEMBERSHIP	1
 #define NETLINK_DROP_MEMBERSHIP	2
 #define NETLINK_PKTINFO		3
+#define NETLINK_BROADCAST_ERROR	4
 
 struct nl_pktinfo
 {
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 6ee69c2..f3f212e 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -85,6 +85,7 @@ struct netlink_sock {
 
 #define NETLINK_KERNEL_SOCKET	0x1
 #define NETLINK_RECV_PKTINFO	0x2
+#define NETLINK_BROADCAST_SEND_ERROR	0x4
 
 static inline struct netlink_sock *nlk_sk(struct sock *sk)
 {
@@ -973,7 +974,7 @@ static inline int do_one_broadcast(struct sock *sk,
 	if (!net_eq(sock_net(sk), p->net))
 		goto out;
 
-	if (p->failure) {
+	if ((nlk->flags & NETLINK_BROADCAST_SEND_ERROR) && p->failure) {
 		netlink_overrun(sk);
 		goto out;
 	}
@@ -994,13 +995,15 @@ static inline int do_one_broadcast(struct sock *sk,
 	if (p->skb2 == NULL) {
 		netlink_overrun(sk);
 		/* Clone failed. Notify ALL listeners. */
-		p->failure = 1;
+		if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
+			p->failure = 1;
 	} else if (sk_filter(sk, p->skb2)) {
 		kfree_skb(p->skb2);
 		p->skb2 = NULL;
 	} else if ((val = netlink_broadcast_deliver(sk, p->skb2)) < 0) {
 		netlink_overrun(sk);
-		p->delivery_failure = 1;
+		if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
+			p->delivery_failure = 1;
 	} else {
 		p->congested |= val;
 		p->delivered = 1;
@@ -1163,6 +1166,13 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		err = 0;
 		break;
 	}
+	case NETLINK_BROADCAST_ERROR:
+		if (val)
+			nlk->flags |= NETLINK_BROADCAST_SEND_ERROR;
+		else
+			nlk->flags &= ~NETLINK_BROADCAST_SEND_ERROR;
+		err = 0;
+		break;
 	default:
 		err = -ENOPROTOOPT;
 	}
@@ -1195,6 +1205,16 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
 			return -EFAULT;
 		err = 0;
 		break;
+	case NETLINK_BROADCAST_ERROR:
+		if (len < sizeof(int))
+			return -EINVAL;
+		len = sizeof(int);
+		val = nlk->flags & NETLINK_BROADCAST_SEND_ERROR ? 1 : 0;
+		if (put_user(len, optlen) ||
+		    put_user(val, optval))
+			return -EFAULT;
+		err = 0;
+		break;
 	default:
 		err = -ENOPROTOOPT;
 	}

Re: [PATCH] netlink: add NETLINK_BROADCAST_ERROR socket option

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> @@ -973,7 +974,7 @@ static inline int do_one_broadcast(struct sock *sk,
>  	if (!net_eq(sock_net(sk), p->net))
>  		goto out;
>  
> -	if (p->failure) {
> +	if ((nlk->flags & NETLINK_BROADCAST_SEND_ERROR) && p->failure) {
>  		netlink_overrun(sk);
>  		goto out;
>  	}
 >
> @@ -994,13 +995,15 @@ static inline int do_one_broadcast(struct sock *sk,
>  	if (p->skb2 == NULL) {
>  		netlink_overrun(sk);
>  		/* Clone failed. Notify ALL listeners. */
> -		p->failure = 1;
> +		if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
> +			p->failure = 1;

Almost :) If we didn't manage to clone, we can't deliver to *any*
socket, so the check in the first chunk above is incorrect. It
needs to always call netlink_overrun(), additionally it needs to
set delivery_failure when the SEND_ERROR flag is present.

Something like this:

  	if (p->failure) {
  +		if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
  +			p->delivery_failure = 1;
   		netlink_overrun(sk);
   		goto out;
   	}

>  	} else if (sk_filter(sk, p->skb2)) {
>  		kfree_skb(p->skb2);
>  		p->skb2 = NULL;
>  	} else if ((val = netlink_broadcast_deliver(sk, p->skb2)) < 0) {
>  		netlink_overrun(sk);
> -		p->delivery_failure = 1;
> +		if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
> +			p->delivery_failure = 1;
>  	} else {
>  		p->congested |= val;
>  		p->delivered = 1;

Re: [PATCH] netlink: add NETLINK_BROADCAST_ERROR socket option

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> @@ -973,7 +974,7 @@ static inline int do_one_broadcast(struct sock *sk,
>>      if (!net_eq(sock_net(sk), p->net))
>>          goto out;
>>  
>> -    if (p->failure) {
>> +    if ((nlk->flags & NETLINK_BROADCAST_SEND_ERROR) && p->failure) {
>>          netlink_overrun(sk);
>>          goto out;
>>      }
>>
>> @@ -994,13 +995,15 @@ static inline int do_one_broadcast(struct sock *sk,
>>      if (p->skb2 == NULL) {
>>          netlink_overrun(sk);
>>          /* Clone failed. Notify ALL listeners. */
>> -        p->failure = 1;
>> +        if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
>> +            p->failure = 1;
> 
> Almost :) If we didn't manage to clone, we can't deliver to *any*
> socket, so the check in the first chunk above is incorrect. It
> needs to always call netlink_overrun(), additionally it needs to
> set delivery_failure when the SEND_ERROR flag is present.

Hm, I'm getting lost with this :), I thought that we agreed that sockets
without the flag set should not skip.

> Something like this:
> 
>      if (p->failure) {
>  +        if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
>  +            p->delivery_failure = 1;

Then, I think that the previous patch that I sent should be OK
(including the flag renaming, of course), because p->delivery_failure is
set if delivery failed and p->failure is set when clone failed. In any
case, (p->failure || p->delivery_failure) results in a error report, so
this would not change anything.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [PATCH] netlink: add NETLINK_BROADCAST_ERROR socket option

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>>> @@ -994,13 +995,15 @@ static inline int do_one_broadcast(struct sock *sk,
>>>      if (p->skb2 == NULL) {
>>>          netlink_overrun(sk);
>>>          /* Clone failed. Notify ALL listeners. */
>>> -        p->failure = 1;
>>> +        if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
>>> +            p->failure = 1;
>> Almost :) If we didn't manage to clone, we can't deliver to *any*
>> socket, so the check in the first chunk above is incorrect. It
>> needs to always call netlink_overrun(), additionally it needs to
>> set delivery_failure when the SEND_ERROR flag is present.
> 
> Hm, I'm getting lost with this :), I thought that we agreed that sockets
> without the flag set should not skip.

Right, but you don't have a choice in that case as you don't
even have the first skb cloned. It will crash.

>> Something like this:
>>
>>      if (p->failure) {
>>  +        if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
>>  +            p->delivery_failure = 1;
> 
> Then, I think that the previous patch that I sent should be OK
> (including the flag renaming, of course), because p->delivery_failure is
> set if delivery failed and p->failure is set when clone failed. In any
> case, (p->failure || p->delivery_failure) results in a error report, so
> this would not change anything.

The last one set p->failure for a deliver failure. It
should set p->delivery_failure, otherwise you *do*
start skipping other sockets.

[PATCH] netlink: add NETLINK_BROADCAST_ERROR socket option

[Pablo Neira Ayuso]

This patch adds NETLINK_BROADCAST_ERROR which is a netlink
socket option that the listener can set to make netlink_broadcast()
return errors in the delivery to the caller. This option is useful
if the caller of netlink_broadcast() do something with the result
of the message delivery, like in ctnetlink where it drops a network
packet if the event delivery failed, this is used to enable reliable
logging and state-synchronization. If this socket option is not set,
netlink_broadcast() only reports ESRCH errors and silently ignore
ENOBUFS errors, which is what most netlink_broadcast() callers
should do.

This socket option is based on a suggestion from Patrick McHardy.
Patrick McHardy can exchange this patch for a beer from me ;).

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---

 include/linux/netlink.h  |    1 +
 net/netlink/af_netlink.c |   25 +++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 51b09a1..1e6bf99 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -103,6 +103,7 @@ struct nlmsgerr
 #define NETLINK_ADD_MEMBERSHIP	1
 #define NETLINK_DROP_MEMBERSHIP	2
 #define NETLINK_PKTINFO		3
+#define NETLINK_BROADCAST_ERROR	4
 
 struct nl_pktinfo
 {
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 6ee69c2..ed587be 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -85,6 +85,7 @@ struct netlink_sock {
 
 #define NETLINK_KERNEL_SOCKET	0x1
 #define NETLINK_RECV_PKTINFO	0x2
+#define NETLINK_BROADCAST_SEND_ERROR	0x4
 
 static inline struct netlink_sock *nlk_sk(struct sock *sk)
 {
@@ -995,12 +996,15 @@ static inline int do_one_broadcast(struct sock *sk,
 		netlink_overrun(sk);
 		/* Clone failed. Notify ALL listeners. */
 		p->failure = 1;
+		if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
+			p->delivery_failure = 1;
 	} else if (sk_filter(sk, p->skb2)) {
 		kfree_skb(p->skb2);
 		p->skb2 = NULL;
 	} else if ((val = netlink_broadcast_deliver(sk, p->skb2)) < 0) {
 		netlink_overrun(sk);
-		p->delivery_failure = 1;
+		if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
+			p->delivery_failure = 1;
 	} else {
 		p->congested |= val;
 		p->delivered = 1;
@@ -1048,7 +1052,7 @@ int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 pid,
 	if (info.skb2)
 		kfree_skb(info.skb2);
 
-	if (info.delivery_failure || info.failure)
+	if (info.delivery_failure)
 		return -ENOBUFS;
 
 	if (info.delivered) {
@@ -1163,6 +1167,13 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		err = 0;
 		break;
 	}
+	case NETLINK_BROADCAST_ERROR:
+		if (val)
+			nlk->flags |= NETLINK_BROADCAST_SEND_ERROR;
+		else
+			nlk->flags &= ~NETLINK_BROADCAST_SEND_ERROR;
+		err = 0;
+		break;
 	default:
 		err = -ENOPROTOOPT;
 	}
@@ -1195,6 +1206,16 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
 			return -EFAULT;
 		err = 0;
 		break;
+	case NETLINK_BROADCAST_ERROR:
+		if (len < sizeof(int))
+			return -EINVAL;
+		len = sizeof(int);
+		val = nlk->flags & NETLINK_BROADCAST_SEND_ERROR ? 1 : 0;
+		if (put_user(len, optlen) ||
+		    put_user(val, optval))
+			return -EFAULT;
+		err = 0;
+		break;
 	default:
 		err = -ENOPROTOOPT;
 	}

Re: [PATCH] netlink: add NETLINK_BROADCAST_ERROR socket option

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> This patch adds NETLINK_BROADCAST_ERROR which is a netlink
> socket option that the listener can set to make netlink_broadcast()
> return errors in the delivery to the caller. This option is useful
> if the caller of netlink_broadcast() do something with the result
> of the message delivery, like in ctnetlink where it drops a network
> packet if the event delivery failed, this is used to enable reliable
> logging and state-synchronization. If this socket option is not set,
> netlink_broadcast() only reports ESRCH errors and silently ignore
> ENOBUFS errors, which is what most netlink_broadcast() callers
> should do.
> 
> This socket option is based on a suggestion from Patrick McHardy.
> Patrick McHardy can exchange this patch for a beer from me ;).

I'd accept that offer, but it needs to go through Dave :)
Anyways, the patch looks good.

Acked-by: Patrick McHardy <kaber@trash.net>

Re: [PATCH] netlink: add NETLINK_BROADCAST_ERROR socket option

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 18 Feb 2009 12:43:36 +0100

> Pablo Neira Ayuso wrote:
> > This patch adds NETLINK_BROADCAST_ERROR which is a netlink
> > socket option that the listener can set to make netlink_broadcast()
> > return errors in the delivery to the caller. This option is useful
> > if the caller of netlink_broadcast() do something with the result
> > of the message delivery, like in ctnetlink where it drops a network
> > packet if the event delivery failed, this is used to enable reliable
> > logging and state-synchronization. If this socket option is not set,
> > netlink_broadcast() only reports ESRCH errors and silently ignore
> > ENOBUFS errors, which is what most netlink_broadcast() callers
> > should do.
> > This socket option is based on a suggestion from Patrick McHardy.
> > Patrick McHardy can exchange this patch for a beer from me ;).
> 
> I'd accept that offer, but it needs to go through Dave :)
> Anyways, the patch looks good.
> 
> Acked-by: Patrick McHardy <kaber@trash.net>

Applied, thanks guys.