RE: [PATCH net-next] change nfqueue failopen to apply also to receive message buffer in addition to queue size

[Yigal Reiss (yreiss)]

Much of the issues raised become redundant due to a much simpler solution 
proposed by Pablo. Still two issues left, proc and potential existing bug in sk 
filter case. 

On March 21, 2016 2:23 PM, Florian Westphal wrote:
> It looks like a bug -- AFAICS if a sk filter is active on the nfnetlink sk we 
> will believe sk got queued and will put the (free'd) skb ptr on the reinject 
> list.
This issue may still need to be looked at separately.

> Userspace already should get -ENOBUFS errors on netlink overrun.
-ENOBUFS doesn't provide statistics. It does not detect queue limitation. Also 
diagnostic needs are many times different than implementation needs. Also many 
times user uses NETLINK_NO_ENOBUFS in order to avoid penalty for buffer 
overruns (as for example done in a patch to daq_nfq submitted by you here 
(http://seclists.org/snort/2011/q2/311) :-) ).

>>  -   seq_printf(s, "%5u %6u %5u %1u %5u %5u %5u %8u %2d\n",
>>  +   seq_printf(s, "%5u %6u %5u %1u %5u %5u %5u %8u %5u %5u %2d\n",
> Problematic since it changes layout of a file we unfortunately have to view 
> as uapi.
> I would prefer if we could leave the proc file alone and not add any new 
> stats counters for this, unless there is a good argument for doing so.
So my arguments are that there in order to fine tune a system it is required to 
know about the existence and number of packets that went under the radar. As I 
wrote ENOBUF does not answer all these needs.
I understand it is problematic to change uapi. Tried to minimize 
incompatibility by keeping the order of arguments. I'll probably use a patch to 
proc any way. Please let me know if you think there is a point in proposing 
this patch or is it a "no-no" from kernel's perspective.

RE: [PATCH net-next] change nfqueue failopen to apply also to receive message buffer in addition to queue size

[Yigal Reiss (yreiss)]

On Monday, March 21, 2016 11:36 PM, Pablo Neira Ayuso wrote:
> So isn't the more simple patch that I'm attaching achieving what you need?
Yes. I applied the patch and it works as expected. Indeed much more simple.

I intend to use this patch and would like it to eventually get into the formal 
kernel. Do you intend to pursue this into mainline kernel? 

[PATCH net-next] change nfqueue failopen to apply also to receive message buffer in addition to queue size

[Yigal Reiss (yreiss)]

Signed-off-by: Yigal Reiss 
---

NOTE: this is a re-send after being bounced by the test robot for a compiler 
warning. I hope I'm doing the right thing in resubmitting rather than replying 
to the original message. Recompiled w/o warnings and re-tested. 

This is follow-up on http://marc.info/?l=netfilter-devel=145765526817347=2 

Existing fail-open mechanism for nfqueue only applies for message that cannot 
be sent due to queue size (queue_maxlen). It does not apply when the failure is 
due to socket receive buffer size. This seems to be quite arbitrary since 
different packet sizes for the same queue and buffer sizes yield failure for 
different reasons.

This patch makes both behave the same. 

There is also a change in the proc file (/proc/net/netfilter/nfnetlink_queue) 
to account for the fail-opened packets.

One change to existing behavior which I would like to stress is in the function 
netlink_unicast (now in netlink_unicast_nofree). In case where a call to 
sk_filter() returned non-zero value, netlink_unicast would set its returned 
error value to skb->len. I don't see how this ever made sense and I couldn't 
find anyone looking at this value. I changed this in order to have a consistent 
(err<0) return value on errors which was required for my changes. If anyone 
sees a problem with this change I'd like to know.  


 include/linux/netfilter/nfnetlink.h |  2 ++
 include/linux/netlink.h |  3 +++
 net/netfilter/nfnetlink.c   |  7 +++
 net/netfilter/nfnetlink_queue.c | 25 ++---
 net/netlink/af_netlink.c| 37 +
 5 files changed, 55 insertions(+), 19 deletions(-)

diff --git a/include/linux/netfilter/nfnetlink.h 
b/include/linux/netfilter/nfnetlink.h
index ba0d978..eb477d4 100644
--- a/include/linux/netfilter/nfnetlink.h
+++ b/include/linux/netfilter/nfnetlink.h
@@ -39,6 +39,8 @@ struct sk_buff *nfnetlink_alloc_skb(struct net *net, unsigned 
int size,
 int nfnetlink_send(struct sk_buff *skb, struct net *net, u32 portid,
   unsigned int group, int echo, gfp_t flags);
 int nfnetlink_set_err(struct net *net, u32 portid, u32 group, int error);
+int nfnetlink_unicast_nofree(struct sk_buff *skb, struct net *net, u32 portid,
+ int flags);
 int nfnetlink_unicast(struct sk_buff *skb, struct net *net, u32 portid,
  int flags);
 
diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 0b41959..9f7a819 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -79,6 +79,7 @@ netlink_alloc_skb(struct sock *ssk, unsigned int size, u32 
dst_portid,
return __netlink_alloc_skb(ssk, size, 0, dst_portid, gfp_mask);
 }
 
+extern int netlink_unicast_nofree(struct sock *ssk, struct sk_buff *skb, __u32 
portid, int nonblock);
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 
portid, int nonblock);
 extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 
portid,
 __u32 group, gfp_t allocation);
@@ -92,6 +93,8 @@ extern int netlink_unregister_notifier(struct notifier_block 
*nb);
 
 /* finegrained unicast helpers: */
 struct sock *netlink_getsockbyfilp(struct file *filp);
+int netlink_attachskb_nofree(struct sock *sk, struct sk_buff *skb,
+ long *timeo, struct sock *ssk);
 int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
  long *timeo, struct sock *ssk);
 void netlink_detachskb(struct sock *sk, struct sk_buff *skb);
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 857ae89..28f7e2d 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -154,6 +154,13 @@ int nfnetlink_unicast(struct sk_buff *skb, struct net 
*net, u32 portid,
 }
 EXPORT_SYMBOL_GPL(nfnetlink_unicast);
 
+int nfnetlink_unicast_nofree(struct sk_buff *skb, struct net *net, u32 portid,
+  int flags)
+{
+return netlink_unicast_nofree(net->nfnl, skb, portid, flags);
+}
+EXPORT_SYMBOL_GPL(nfnetlink_unicast_nofree);
+
 /* Process one complete nfnetlink message. */
 static int nfnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 {
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 1d39365..3d32153 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -60,7 +60,8 @@ struct nfqnl_instance {
unsigned int copy_range;
unsigned int queue_dropped;
unsigned int queue_user_dropped;
-
+unsigned int queue_failopened;
+unsigned int nobuf_failopened;
 
u_int16_t queue_num;/* number of this queue */
u_int8_t copy_mode;
@@ -551,6 +552,7 @@ nla_put_failure:
return NULL;
 }
 
+
 static int
 __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
struct nf_queue_entry *entry)
@@ -569,6 +571,7 @@ 

[PATCH] change nfqueue fail-open mechanism to apply also to receive message

[Yigal Reiss (yreiss)]

Signed-off-by: Yigal Reiss 
---

This is follow-up on http://marc.info/?l=netfilter-devel=145765526817347=2 

Existing fail-open mechanism for nfqueue only applies for message that cannot 
be sent due to queue size (queue_maxlen). It does not apply when the failure is 
due to socket receive buffer size. This seems to be quite arbitrary since 
different packet sizes for the same queue and buffer sizes yield failure for 
different reasons.

This patch makes both behave the same. 

There is also a change in the proc file (/proc/net/netfilter/nfnetlink_queue) 
to account for the fail-opened packets.

One change to existing behavior which I would like to stress is in the function 
netlink_unicast (now in netlink_unicast_nofree). In case where a call to 
sk_filter() returned non-zero value, netlink_unicast would set its returned 
error value to skb->len. I don't see how this ever made sense and I couldn't 
find anyone looking at this value. I changed this in order to have a consistent 
(err<0) return value on errors which was required for my changes. If anyone 
sees a problem with this change I'd like to know.  


 include/linux/netfilter/nfnetlink.h |  2 ++
 include/linux/netlink.h |  3 +++
 net/netfilter/nfnetlink.c   |  7 +++
 net/netfilter/nfnetlink_queue.c | 25 ++---
 net/netlink/af_netlink.c| 36 
 5 files changed, 54 insertions(+), 19 deletions(-)

diff --git a/include/linux/netfilter/nfnetlink.h 
b/include/linux/netfilter/nfnetlink.h
index ba0d978..eb477d4 100644
--- a/include/linux/netfilter/nfnetlink.h
+++ b/include/linux/netfilter/nfnetlink.h
@@ -39,6 +39,8 @@ struct sk_buff *nfnetlink_alloc_skb(struct net *net, unsigned 
int size,
 int nfnetlink_send(struct sk_buff *skb, struct net *net, u32 portid,
   unsigned int group, int echo, gfp_t flags);
 int nfnetlink_set_err(struct net *net, u32 portid, u32 group, int error);
+int nfnetlink_unicast_nofree(struct sk_buff *skb, struct net *net, u32 portid,
+ int flags);
 int nfnetlink_unicast(struct sk_buff *skb, struct net *net, u32 portid,
  int flags);
 
diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 0b41959..9f7a819 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -79,6 +79,7 @@ netlink_alloc_skb(struct sock *ssk, unsigned int size, u32 
dst_portid,
return __netlink_alloc_skb(ssk, size, 0, dst_portid, gfp_mask);
 }
 
+extern int netlink_unicast_nofree(struct sock *ssk, struct sk_buff *skb, __u32 
portid, int nonblock);
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 
portid, int nonblock);
 extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 
portid,
 __u32 group, gfp_t allocation);
@@ -92,6 +93,8 @@ extern int netlink_unregister_notifier(struct notifier_block 
*nb);
 
 /* finegrained unicast helpers: */
 struct sock *netlink_getsockbyfilp(struct file *filp);
+int netlink_attachskb_nofree(struct sock *sk, struct sk_buff *skb,
+ long *timeo, struct sock *ssk);
 int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
  long *timeo, struct sock *ssk);
 void netlink_detachskb(struct sock *sk, struct sk_buff *skb);
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 857ae89..28f7e2d 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -154,6 +154,13 @@ int nfnetlink_unicast(struct sk_buff *skb, struct net 
*net, u32 portid,
 }
 EXPORT_SYMBOL_GPL(nfnetlink_unicast);
 
+int nfnetlink_unicast_nofree(struct sk_buff *skb, struct net *net, u32 portid,
+  int flags)
+{
+return netlink_unicast_nofree(net->nfnl, skb, portid, flags);
+}
+EXPORT_SYMBOL_GPL(nfnetlink_unicast_nofree);
+
 /* Process one complete nfnetlink message. */
 static int nfnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 {
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 1d39365..3d32153 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -60,7 +60,8 @@ struct nfqnl_instance {
unsigned int copy_range;
unsigned int queue_dropped;
unsigned int queue_user_dropped;
-
+unsigned int queue_failopened;
+unsigned int nobuf_failopened;
 
u_int16_t queue_num;/* number of this queue */
u_int8_t copy_mode;
@@ -551,6 +552,7 @@ nla_put_failure:
return NULL;
 }
 
+
 static int
 __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
struct nf_queue_entry *entry)
@@ -569,6 +571,7 @@ __nfqnl_enqueue_packet(struct net *net, struct 
nfqnl_instance *queue,
 
if (queue->queue_total >= queue->queue_maxlen) {
if (queue->flags & NFQA_CFG_F_FAIL_OPEN) {
+   

[PATCH] brouted packet identified as PACKET_OTHERHOST blocked by higher protocol

[Yigal Reiss (yreiss)]

The problem I'm trying to solve is that when packets being sent from one 
bridged interface to the other are brouted they get dropped by the IP layer. 
The reason is that the packet being raised has pkt_type of type 
PACKET_OTHERHOST.

The semantics of brouting a packet is that it is sent up to higher network 
layers. Problem is that when the pkt_type of the packet is 
PACKET_OTHERHOST it (at least for IP) gets dropped. (e.g. dropping OTHERHOST 
packets is the first thing ip_rcv() does).

The suggested patch below changes the packet type to PACKET_HOST which fixes 
the problem. This is a bit of a cheat but I didn't find any side effects and 
couldn't find a better way w/o defining a new packet type. 

Also removed  dest = eth_hdr(skb)-h_dest; which does nothing as far as I can 
see as it is already assigned the same value before.

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index f921a5d..2cae324 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -291,12 +291,11 @@ forward:
switch (p-state) {
case BR_STATE_FORWARDING:
rhook = rcu_dereference(br_should_route_hook);
-   if (rhook) {
-   if ((*rhook)(skb)) {
-   *pskb = skb;
-   return RX_HANDLER_PASS;
-   }
-   dest = eth_hdr(skb)-h_dest;
+   if (rhook  (*rhook)(skb)) {
+   if (skb-pkt_type == PACKET_OTHERHOST)
+   skb-pkt_type = PACKET_HOST; /* so it does not get 
rejected by higher protocol receiver, e.g. by ip_rcv()  */
+   *pskb = skb;
+   return RX_HANDLER_PASS;
}
/* fall through */
case BR_STATE_LEARNING:
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] brouted packet identified as PACKET_OTHERHOST blocked by higher protocol

[Yigal Reiss (yreiss)]

Florian Westphal [mailto:f...@strlen.de] wrote:
 Maybe, but if you broute everything you might as well just remove the
 bridge...
I want to be selective. My setup is a home router. So I can have ebtables rules 
for 
which traffic to (b)route and which to bridge, based on security/performance 
criteria.

 You can use -j redirect in ebtables broute table to force local MAC dnat
 (this also 'fixes' the pkttype to _HOST) if you really want to broute.
I may be missing something obvious, but what is the normal case where using an 
ebtables 'broute' -j DROP rule does work? It seemed to me that without the 
fix all (b)routed packets would get lost in IP layer
(unless also dnat or something is done in addition which changes the pkt_type 
value). 
What is the original intention of this table/chain if not pulling packets 
between 
other hosts out of the bridge and passing them through the IP and higher 
layers?
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] brouted packet identified as PACKET_OTHERHOST blocked by higher protocol

[Yigal Reiss (yreiss)]

Florian Westphal f...@strlen.de wrote:
 Yigal Reiss (yreiss) yre...@cisco.com wrote:
  The problem I'm trying to solve is that when packets being sent from
 one bridged interface to the other are brouted they get dropped by the
 IP layer. The reason is that the packet being raised has pkt_type of
 type PACKET_OTHERHOST.
 
 No, thats not the problem you're trying to solve.
 
 If you want to move OTHERHOST skbs, don't (b)route them?
 
 Whats the real issue that you're trying to solve?

I want to (b)route them because I want to be able to inspect the packets in 
higher levels
(through iptables or user space IPS).

Once I do that (i.e. (b)route by applying an appropriate ebtables rule), the 
corresponding
packets get dropped unless I apply the patch.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html