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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 14 Oct 2014 22:51:32 +0800
From:	Houcheng Lin <houcheng@...il.com>
To:	Florian Westphal <fw@...len.de>
Cc:	pablo@...filter.org, Patrick McHardy <kaber@...sh.net>,
	kadlec@...ckhole.kfki.hu, davem@...emloft.net,
	netfilter-devel@...r.kernel.org, coreteam@...filter.org,
	netdev@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] netfilter: release skbuf when nlmsg put fail

2014-10-14 18:49 GMT+08:00 Florian Westphal <fw@...len.de>:
> Houcheng Lin <houcheng@...il.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/