* [PATCH 1/3] netfilter: nfnetlink_log: remove unnecessary error messages @ 2014-11-06 11:32 Pablo Neira Ayuso 2014-11-06 11:32 ` [PATCH 2/3] netfilter: nfnetlink_log: improve error handling on __build_packet_message() Pablo Neira Ayuso 2014-11-06 11:32 ` [PATCH 3/3] netfilter: nfnetlink_log: Make use of pr_fmt where applicable Pablo Neira Ayuso 0 siblings, 2 replies; 4+ messages in thread From: Pablo Neira Ayuso @ 2014-11-06 11:32 UTC (permalink / raw) To: netfilter-devel; +Cc: fw, mleitner In case of OOM, there's nothing userspace can do. If there's no room to put the payload in __build_packet_message(), jump to nla_put_failure which already performs the corresponding error reporting. Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- net/netfilter/nfnetlink_log.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c index 5f1be5b..cd99294 100644 --- a/net/netfilter/nfnetlink_log.c +++ b/net/netfilter/nfnetlink_log.c @@ -337,9 +337,6 @@ nfulnl_alloc_skb(struct net *net, u32 peer_portid, unsigned int inst_size, skb = nfnetlink_alloc_skb(net, pkt_size, peer_portid, GFP_ATOMIC); - if (!skb) - pr_err("nfnetlink_log: can't even alloc %u bytes\n", - pkt_size); } } @@ -570,10 +567,8 @@ __build_packet_message(struct nfnl_log_net *log, struct nlattr *nla; int size = nla_attr_size(data_len); - if (skb_tailroom(inst->skb) < nla_total_size(data_len)) { - printk(KERN_WARNING "nfnetlink_log: no tailroom!\n"); - return -1; - } + if (skb_tailroom(inst->skb) < nla_total_size(data_len)) + goto nla_put_failure; nla = (struct nlattr *)skb_put(inst->skb, nla_total_size(data_len)); nla->nla_type = NFULA_PAYLOAD; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/3] netfilter: nfnetlink_log: improve error handling on __build_packet_message() 2014-11-06 11:32 [PATCH 1/3] netfilter: nfnetlink_log: remove unnecessary error messages Pablo Neira Ayuso @ 2014-11-06 11:32 ` Pablo Neira Ayuso 2014-11-06 17:19 ` Marcelo Ricardo Leitner 2014-11-06 11:32 ` [PATCH 3/3] netfilter: nfnetlink_log: Make use of pr_fmt where applicable Pablo Neira Ayuso 1 sibling, 1 reply; 4+ messages in thread From: Pablo Neira Ayuso @ 2014-11-06 11:32 UTC (permalink / raw) To: netfilter-devel; +Cc: fw, mleitner 1) If there's no enough room in the netlink skbuff, then we have a size miscalculation bug that needs to be fixed, so warn on this. Kill PRINTR macro now that this is unused. 2) Cancel the netlink message that didn't fit into the skbuff, so we still have the chance to deliver what is already included in the batch. 3) Don't increment inst->qlen inconditionally. Otherwise, this will not show the real number of messages in the log batch on error. Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- net/netfilter/nfnetlink_log.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c index cd99294..551142f 100644 --- a/net/netfilter/nfnetlink_log.c +++ b/net/netfilter/nfnetlink_log.c @@ -46,9 +46,6 @@ /* max packet size is limited by 16-bit struct nfattr nfa_len field */ #define NFULNL_COPY_RANGE_MAX (0xFFFF - NLA_HDRLEN) -#define PRINTR(x, args...) do { if (net_ratelimit()) \ - printk(x, ## args); } while (0); - struct nfulnl_instance { struct hlist_node hlist; /* global list of instances */ spinlock_t lock; @@ -402,7 +399,6 @@ __build_packet_message(struct nfnl_log_net *log, struct nfulnl_msg_packet_hdr pmsg; struct nlmsghdr *nlh; struct nfgenmsg *nfmsg; - sk_buff_data_t old_tail = inst->skb->tail; struct sock *sk; const unsigned char *hwhdrp; @@ -578,11 +574,15 @@ __build_packet_message(struct nfnl_log_net *log, BUG(); } - nlh->nlmsg_len = inst->skb->tail - old_tail; + nlmsg_end(inst->skb, nlh); + inst->qlen++; + return 0; nla_put_failure: - PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n"); + WARN_ONCE(1, "bad nlskb size: %u, tailroom %d\n", + inst->skb->len, skb_tailroom(inst->skb)); + nlmsg_cancel(inst->skb, nlh); return -1; } @@ -702,8 +702,6 @@ nfulnl_log_packet(struct net *net, goto alloc_failure; } - inst->qlen++; - __build_packet_message(log, inst, skb, data_len, pf, hooknum, in, out, prefix, plen); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/3] netfilter: nfnetlink_log: improve error handling on __build_packet_message() 2014-11-06 11:32 ` [PATCH 2/3] netfilter: nfnetlink_log: improve error handling on __build_packet_message() Pablo Neira Ayuso @ 2014-11-06 17:19 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 4+ messages in thread From: Marcelo Ricardo Leitner @ 2014-11-06 17:19 UTC (permalink / raw) To: Pablo Neira Ayuso, netfilter-devel; +Cc: fw On 06-11-2014 09:32, Pablo Neira Ayuso wrote: > 1) If there's no enough room in the netlink skbuff, then we have a size > miscalculation bug that needs to be fixed, so warn on this. Kill > PRINTR macro now that this is unused. > > 2) Cancel the netlink message that didn't fit into the skbuff, so we still > have the chance to deliver what is already included in the batch. > > 3) Don't increment inst->qlen inconditionally. Otherwise, this will not > show the real number of messages in the log batch on error. > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > net/netfilter/nfnetlink_log.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c > index cd99294..551142f 100644 > --- a/net/netfilter/nfnetlink_log.c > +++ b/net/netfilter/nfnetlink_log.c > @@ -46,9 +46,6 @@ > /* max packet size is limited by 16-bit struct nfattr nfa_len field */ > #define NFULNL_COPY_RANGE_MAX (0xFFFF - NLA_HDRLEN) > > -#define PRINTR(x, args...) do { if (net_ratelimit()) \ > - printk(x, ## args); } while (0); > - > struct nfulnl_instance { > struct hlist_node hlist; /* global list of instances */ > spinlock_t lock; > @@ -402,7 +399,6 @@ __build_packet_message(struct nfnl_log_net *log, > struct nfulnl_msg_packet_hdr pmsg; > struct nlmsghdr *nlh; > struct nfgenmsg *nfmsg; > - sk_buff_data_t old_tail = inst->skb->tail; > struct sock *sk; > const unsigned char *hwhdrp; > > @@ -578,11 +574,15 @@ __build_packet_message(struct nfnl_log_net *log, > BUG(); > } > > - nlh->nlmsg_len = inst->skb->tail - old_tail; > + nlmsg_end(inst->skb, nlh); > + inst->qlen++; > + > return 0; > > nla_put_failure: > - PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n"); > + WARN_ONCE(1, "bad nlskb size: %u, tailroom %d\n", > + inst->skb->len, skb_tailroom(inst->skb)); > + nlmsg_cancel(inst->skb, nlh); > return -1; > } > > @@ -702,8 +702,6 @@ nfulnl_log_packet(struct net *net, > goto alloc_failure; > } > > - inst->qlen++; > - > __build_packet_message(log, inst, skb, data_len, pf, > hooknum, in, out, prefix, plen); I would prefer if we freed the skbuff if __build_package_message failed, so that the next one is allocated with other params. Also, if it's the first message, we may fire the timer for delivering an empty message, as the timer is going to be scheduled and inst->skb is still there. Maybe with: @@ -707,16 +707,19 @@ nfulnl_log_packet(struct net *net, goto alloc_failure; } - inst->qlen++; - - __build_packet_message(log, inst, skb, data_len, pf, - hooknum, in, out, prefix, plen); + if (__build_packet_message(log, inst, skb, data_len, pf, + hooknum, in, out, prefix, plen)) { + if (!inst->qlen) { + kfree_skb(inst->skb); + inst->skb = NULL; + } + } if (inst->qlen >= qthreshold) __nfulnl_flush(inst); /* timer_pending always called within inst->lock, so there * is no chance of a race here */ - else if (!timer_pending(&inst->timer)) { + else if (inst->qlen && !timer_pending(&inst->timer)) { instance_get(inst); inst->timer.expires = jiffies + (inst->flushtimeout*HZ/100); add_timer(&inst->timer); but that still wouldn't force a flush if __build_packet_message() failed.. Other than that, set looks great to me. Thanks Pablo! Marcelo ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 3/3] netfilter: nfnetlink_log: Make use of pr_fmt where applicable 2014-11-06 11:32 [PATCH 1/3] netfilter: nfnetlink_log: remove unnecessary error messages Pablo Neira Ayuso 2014-11-06 11:32 ` [PATCH 2/3] netfilter: nfnetlink_log: improve error handling on __build_packet_message() Pablo Neira Ayuso @ 2014-11-06 11:32 ` Pablo Neira Ayuso 1 sibling, 0 replies; 4+ messages in thread From: Pablo Neira Ayuso @ 2014-11-06 11:32 UTC (permalink / raw) To: netfilter-devel; +Cc: fw, mleitner From: Marcelo Leitner <mleitner@redhat.com> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- net/netfilter/nfnetlink_log.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c index 551142f..66a2f4c 100644 --- a/net/netfilter/nfnetlink_log.c +++ b/net/netfilter/nfnetlink_log.c @@ -12,6 +12,9 @@ * it under the terms of the GNU General Public License version 2 as * published by the Free Software Foundation. */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + #include <linux/module.h> #include <linux/skbuff.h> #include <linux/if_arp.h> @@ -1062,19 +1065,19 @@ static int __init nfnetlink_log_init(void) netlink_register_notifier(&nfulnl_rtnl_notifier); status = nfnetlink_subsys_register(&nfulnl_subsys); if (status < 0) { - pr_err("log: failed to create netlink socket\n"); + pr_err("failed to create netlink socket\n"); goto cleanup_netlink_notifier; } status = nf_log_register(NFPROTO_UNSPEC, &nfulnl_logger); if (status < 0) { - pr_err("log: failed to register logger\n"); + pr_err("failed to register logger\n"); goto cleanup_subsys; } status = register_pernet_subsys(&nfnl_log_net_ops); if (status < 0) { - pr_err("log: failed to register pernet ops\n"); + pr_err("failed to register pernet ops\n"); goto cleanup_logger; } return status; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-11-06 17:19 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-06 11:32 [PATCH 1/3] netfilter: nfnetlink_log: remove unnecessary error messages Pablo Neira Ayuso 2014-11-06 11:32 ` [PATCH 2/3] netfilter: nfnetlink_log: improve error handling on __build_packet_message() Pablo Neira Ayuso 2014-11-06 17:19 ` Marcelo Ricardo Leitner 2014-11-06 11:32 ` [PATCH 3/3] netfilter: nfnetlink_log: Make use of pr_fmt where applicable Pablo Neira Ayuso
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.