diff mbox

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

Message ID 20160321213532.GA1818@salvia
State Not Applicable
Delegated to: Pablo Neira
Headers show

Commit Message

Pablo Neira Ayuso March 21, 2016, 9:35 p.m. UTC 
On Mon, Mar 21, 2016 at 11:23:43AM +0000, Yigal Reiss (yreiss) wrote:
> @@ -582,10 +585,17 @@ __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
>  	*packet_id_ptr = htonl(entry->id);
>  
>  	/* nfnetlink_unicast will either free the nskb or add it to a socket */
> -	err = nfnetlink_unicast(nskb, net, queue->peer_portid, MSG_DONTWAIT);
> +	err = nfnetlink_unicast_nofree(nskb, net, queue->peer_portid, MSG_DONTWAIT);

This keeps nskb around, this skbuff contains the netlink message, not
the network packet itself that is located in entry->skb.

>  	if (err < 0) {
> -		queue->queue_user_dropped++;
> -		goto err_out_unlock;
> +		if (queue->flags & NFQA_CFG_F_FAIL_OPEN) {
> +		        queue->nobuf_failopened++;
> +		        failopen = 1;
> +			err = 0;

In case we couldn't deliver due to socket buffer overrun, if the
NFQA_CFG_F_FAIL_OPEN flag is set, you set failopen to 1.

> +		}
> +		else {
> +		    queue->queue_user_dropped++;
> +		}
> +		goto err_out_free_nskb;

And finally, jump to err_out_free_nskb.

>  	}
>  
>  	__enqueue_entry(queue, entry);
> @@ -595,7 +605,6 @@ __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
>  
>  err_out_free_nskb:
>  	kfree_skb(nskb);

Which just releases the netlink skbuff.

> -err_out_unlock:
>  	spin_unlock_bh(&queue->lock);
>  	if (failopen)
>  		nf_reinject(entry, NF_ACCEPT);

And reinjects the packet.

So isn't the more simple patch that I'm attaching achieving what you need?

Let me know, thanks.

Comments

Yigal Reiss (yreiss) March 23, 2016, 11:40 a.m. UTC | #1 
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? 

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso March 23, 2016, 11:58 a.m. UTC | #2 
On Wed, Mar 23, 2016 at 11:40:14AM +0000, Yigal Reiss (yreiss) wrote:
> 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?

Just made a formal submission of the patch.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 7542999..cb5b630 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -582,7 +582,12 @@  __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
 	/* nfnetlink_unicast will either free the nskb or add it to a socket */
 	err = nfnetlink_unicast(nskb, net, queue->peer_portid, MSG_DONTWAIT);
 	if (err < 0) {
-		queue->queue_user_dropped++;
+		if (queue->flags & NFQA_CFG_F_FAIL_OPEN) {
+			failopen = 1;
+			err = 0;
+		} else {
+			queue->queue_user_dropped++;
+		}
 		goto err_out_unlock;
 	}