[PATCH v2] netfilter: release skbuf when nlmsg put fail

[PATCH v2] netfilter: release skbuf when nlmsg put fail

[Houcheng Lin]

When system is under heavy loading, the __nfulnl_send() may may failed
to put nlmsg into skbuf of nfulnl_instance. If not clear the skbuff on failed,
the __nfulnl_send() will still try to put next nlmsg onto this half-full skbuf
and cause the user program can never receive packet.

This patch fix this issue by releasing skbuf immediately after nlmst put
failed.

Signed-off-by: Houcheng Lin <houcheng@gmail.com>
---
 net/netfilter/nfnetlink_log.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index a11c5ff..0ad5d32 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -353,15 +353,17 @@ __nfulnl_send(struct nfulnl_instance *inst)
  NLMSG_DONE,
  sizeof(struct nfgenmsg),
  0);
- if (!nlh)
+ if (!nlh) {
+ WARN_ON(1);
+ kfree_skb(inst->skb);
  goto out;
+ }
  }
  status = nfnetlink_unicast(inst->skb, inst->net, inst->peer_portid,
    MSG_DONTWAIT);
-
+out:
  inst->qlen = 0;
  inst->skb = NULL;
-out:
  return status;
 }

-- 
1.7.9.5

Re: [PATCH v2] netfilter: release skbuf when nlmsg put fail

[Florian Westphal]

Houcheng Lin <houcheng@gmail.com> wrote:
> When system is under heavy loading, the __nfulnl_send() may may failed
> to put nlmsg into skbuf of nfulnl_instance. If not clear the skbuff on failed,
> the __nfulnl_send() will still try to put next nlmsg onto this half-full skbuf
> and cause the user program can never receive packet.
> 
> This patch fix this issue by releasing skbuf immediately after nlmst put
> failed.

Could you please try this patch on top of this one and see if the
WARN_ON goes away?

Thanks

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -649,7 +649,8 @@ nfulnl_log_packet(struct net *net,
 		+ nla_total_size(sizeof(u_int32_t))	/* gid */
 		+ nla_total_size(plen)			/* prefix */
 		+ nla_total_size(sizeof(struct nfulnl_msg_packet_hw))
-		+ nla_total_size(sizeof(struct nfulnl_msg_packet_timestamp));
+		+ nla_total_size(sizeof(struct nfulnl_msg_packet_timestamp))
+		+ nla_total_size(sizeof(struct nfgenmsg));	/* NLMSG_DONE */
 
 	if (in && skb_mac_header_was_set(skb)) {
 		size +=   nla_total_size(skb->dev->hard_header_len)
@@ -692,8 +693,7 @@ nfulnl_log_packet(struct net *net,
 		goto unlock_and_release;
 	}
 
-	if (inst->skb &&
-	    size > skb_tailroom(inst->skb) - sizeof(struct nfgenmsg)) {
+	if (inst->skb && size > skb_tailroom(inst->skb)) {
 		/* either the queue len is too high or we don't have
 		 * enough room in the skb left. flush to userspace. */
 		__nfulnl_flush(inst);

Re: [PATCH v2] netfilter: release skbuf when nlmsg put fail

[Houcheng Lin]

2014-10-14 18:49 GMT+08:00 Florian Westphal <fw@strlen.de>:
> Houcheng Lin <houcheng@gmail.com> wrote:
>> When system is under heavy loading, the __nfulnl_send() may may failed
>> to put nlmsg into skbuf of nfulnl_instance. If not clear the skbuff on failed,
>> the __nfulnl_send() will still try to put next nlmsg onto this half-full skbuf
>> and cause the user program can never receive packet.
>>
>> This patch fix this issue by releasing skbuf immediately after nlmst put
>> failed.
>
> Could you please try this patch on top of this one and see if the
> WARN_ON goes away?
>
> Thanks
>
> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
> --- a/net/netfilter/nfnetlink_log.c
> +++ b/net/netfilter/nfnetlink_log.c
> @@ -649,7 +649,8 @@ nfulnl_log_packet(struct net *net,
>                 + nla_total_size(sizeof(u_int32_t))     /* gid */
>                 + nla_total_size(plen)                  /* prefix */
>                 + nla_total_size(sizeof(struct nfulnl_msg_packet_hw))
> -               + nla_total_size(sizeof(struct nfulnl_msg_packet_timestamp));
> +               + nla_total_size(sizeof(struct nfulnl_msg_packet_timestamp))
> +               + nla_total_size(sizeof(struct nfgenmsg));      /* NLMSG_DONE */
>
>         if (in && skb_mac_header_was_set(skb)) {
>                 size +=   nla_total_size(skb->dev->hard_header_len)
> @@ -692,8 +693,7 @@ nfulnl_log_packet(struct net *net,
>                 goto unlock_and_release;
>         }
>
> -       if (inst->skb &&
> -           size > skb_tailroom(inst->skb) - sizeof(struct nfgenmsg)) {
> +       if (inst->skb && size > skb_tailroom(inst->skb)) {
>                 /* either the queue len is too high or we don't have
>                  * enough room in the skb left. flush to userspace. */
>                 __nfulnl_flush(inst);
Hi Florian,
The modified code seems won't affect the program flow: Size is add a
extra value,
sizeof(struct nfgenmsg), during initialization. comparison size with tailroom
space, the right-side value also add the same value. Is there anything
I miss or
not understand ?

-- 
Best regards,
Houcheng Lin

Re: [PATCH v2] netfilter: release skbuf when nlmsg put fail

[Florian Westphal]

Houcheng Lin <houcheng@gmail.com> wrote:
> 2014-10-14 18:49 GMT+08:00 Florian Westphal <fw@strlen.de>:
> > Houcheng Lin <houcheng@gmail.com> wrote:
> >> When system is under heavy loading, the __nfulnl_send() may may failed
> >> to put nlmsg into skbuf of nfulnl_instance. If not clear the skbuff on failed,
> >> the __nfulnl_send() will still try to put next nlmsg onto this half-full skbuf
> >> and cause the user program can never receive packet.
> >>
> >> This patch fix this issue by releasing skbuf immediately after nlmst put
> >> failed.
> >
> > Could you please try this patch on top of this one and see if the
> > WARN_ON goes away?
> >
> > Thanks
> >
> > diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
> > --- a/net/netfilter/nfnetlink_log.c
> > +++ b/net/netfilter/nfnetlink_log.c
> > @@ -649,7 +649,8 @@ nfulnl_log_packet(struct net *net,
> >                 + nla_total_size(sizeof(u_int32_t))     /* gid */
> >                 + nla_total_size(plen)                  /* prefix */
> >                 + nla_total_size(sizeof(struct nfulnl_msg_packet_hw))
> > -               + nla_total_size(sizeof(struct nfulnl_msg_packet_timestamp));
> > +               + nla_total_size(sizeof(struct nfulnl_msg_packet_timestamp))
> > +               + nla_total_size(sizeof(struct nfgenmsg));      /* NLMSG_DONE */
> >
> >         if (in && skb_mac_header_was_set(skb)) {
> >                 size +=   nla_total_size(skb->dev->hard_header_len)
> > @@ -692,8 +693,7 @@ nfulnl_log_packet(struct net *net,
> >                 goto unlock_and_release;
> >         }
> >
> > -       if (inst->skb &&
> > -           size > skb_tailroom(inst->skb) - sizeof(struct nfgenmsg)) {
> > +       if (inst->skb && size > skb_tailroom(inst->skb)) {
> >                 /* either the queue len is too high or we don't have
> >                  * enough room in the skb left. flush to userspace. */
> >                 __nfulnl_flush(inst);
> Hi Florian,
> The modified code seems won't affect the program flow: Size is add a
> extra value,
> sizeof(struct nfgenmsg), during initialization. comparison size with tailroom
> space, the right-side value also add the same value.

There are two changes:

sizeof(struct nfgenmsg) is not the same as nla_total_size(sizeof(struct
nfgenmsg)).

Also, adding it means we consider the DONE space when allocationg the
skb.