| Message ID | 65a78d556ae1682fdf38f8bdb424204ee72d1ad9.1414175014.git.mleitner@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Pablo Neira |
| Headers | show |
On Fri, Oct 24, 2014 at 04:46:32PM -0200, Marcelo Ricardo Leitner wrote: > And also remove PRINTR macro in favor of net_*_ratelimited(). > > Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com> > --- > > Notes: > v1-v2: > make use of net_err_ratelimited() > > net/netfilter/nfnetlink_log.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c > index b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..0eb10bf295e92ad29541f58ca1de67e3380157c0 100644 > --- a/net/netfilter/nfnetlink_log.c > +++ b/net/netfilter/nfnetlink_log.c > @@ -12,6 +12,8 @@ > * 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> > @@ -45,9 +47,6 @@ > #define NFULNL_QTHRESH_DEFAULT 100 /* 100 packets */ > #define NFULNL_COPY_RANGE_MAX 0xFFFF /* max packet size is limited by 16-bit struct nfattr nfa_len field */ > > -#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; > @@ -335,8 +334,7 @@ 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); > + pr_err("can't even alloc %u bytes\n", pkt_size); > } > } > > @@ -569,7 +567,7 @@ __build_packet_message(struct nfnl_log_net *log, > 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"); > + pr_warn("no tailroom!\n"); > return -1; I think we can just goto nla_put_failure and remove this warning. > } > > @@ -585,7 +583,7 @@ __build_packet_message(struct nfnl_log_net *log, > return 0; > > nla_put_failure: > - PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n"); > + net_err_ratelimited("error creating log nlmsg\n"); I'd remove this one too. But before doing any error message cleanup, we should also call nlmsg_cancel() here to leave the nflog netlink batch message in consistent state in case of errors. And we don't check for errors after __build_packet_message() which may result in sending an incomplete message to userspace. Would you send us patches to address this? Thanks. -- 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
On 27-10-2014 20:23, Pablo Neira Ayuso wrote: > On Fri, Oct 24, 2014 at 04:46:32PM -0200, Marcelo Ricardo Leitner wrote: >> And also remove PRINTR macro in favor of net_*_ratelimited(). >> >> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com> >> --- >> >> Notes: >> v1-v2: >> make use of net_err_ratelimited() >> >> net/netfilter/nfnetlink_log.c | 18 ++++++++---------- >> 1 file changed, 8 insertions(+), 10 deletions(-) >> >> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c >> index b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..0eb10bf295e92ad29541f58ca1de67e3380157c0 100644 >> --- a/net/netfilter/nfnetlink_log.c >> +++ b/net/netfilter/nfnetlink_log.c >> @@ -12,6 +12,8 @@ >> * 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> >> @@ -45,9 +47,6 @@ >> #define NFULNL_QTHRESH_DEFAULT 100 /* 100 packets */ >> #define NFULNL_COPY_RANGE_MAX 0xFFFF /* max packet size is limited by 16-bit struct nfattr nfa_len field */ >> >> -#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; >> @@ -335,8 +334,7 @@ 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); >> + pr_err("can't even alloc %u bytes\n", pkt_size); >> } >> } >> >> @@ -569,7 +567,7 @@ __build_packet_message(struct nfnl_log_net *log, >> 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"); >> + pr_warn("no tailroom!\n"); >> return -1; > > I think we can just goto nla_put_failure and remove this warning. ok >> } >> >> @@ -585,7 +583,7 @@ __build_packet_message(struct nfnl_log_net *log, >> return 0; >> >> nla_put_failure: >> - PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n"); >> + net_err_ratelimited("error creating log nlmsg\n"); > > I'd remove this one too. > > But before doing any error message cleanup, we should also call > nlmsg_cancel() here to leave the nflog netlink batch message in > consistent state in case of errors. And we don't check for errors > after __build_packet_message() which may result in sending an > incomplete message to userspace. okay > Would you send us patches to address this? Thanks. Sure thing. Thanks Pablo. Marcelo -- 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
On 28-10-2014 10:59, Marcelo Ricardo Leitner wrote: > On 27-10-2014 20:23, Pablo Neira Ayuso wrote: >> On Fri, Oct 24, 2014 at 04:46:32PM -0200, Marcelo Ricardo Leitner wrote: >>> And also remove PRINTR macro in favor of net_*_ratelimited(). >>> >>> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com> >>> --- >>> >>> Notes: >>> v1-v2: >>> make use of net_err_ratelimited() >>> >>> net/netfilter/nfnetlink_log.c | 18 ++++++++---------- >>> 1 file changed, 8 insertions(+), 10 deletions(-) >>> >>> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c >>> index >>> b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..0eb10bf295e92ad29541f58ca1de67e3380157c0 >>> 100644 >>> --- a/net/netfilter/nfnetlink_log.c >>> +++ b/net/netfilter/nfnetlink_log.c >>> @@ -12,6 +12,8 @@ >>> * 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> >>> @@ -45,9 +47,6 @@ >>> #define NFULNL_QTHRESH_DEFAULT 100 /* 100 packets */ >>> #define NFULNL_COPY_RANGE_MAX 0xFFFF /* max packet size is limited >>> by 16-bit struct nfattr nfa_len field */ >>> >>> -#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; >>> @@ -335,8 +334,7 @@ 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); >>> + pr_err("can't even alloc %u bytes\n", pkt_size); >>> } >>> } >>> >>> @@ -569,7 +567,7 @@ __build_packet_message(struct nfnl_log_net *log, >>> 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"); >>> + pr_warn("no tailroom!\n"); >>> return -1; >> >> I think we can just goto nla_put_failure and remove this warning. > > ok > >>> } >>> >>> @@ -585,7 +583,7 @@ __build_packet_message(struct nfnl_log_net *log, >>> return 0; >>> >>> nla_put_failure: >>> - PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n"); >>> + net_err_ratelimited("error creating log nlmsg\n"); >> >> I'd remove this one too. >> >> But before doing any error message cleanup, we should also call >> nlmsg_cancel() here to leave the nflog netlink batch message in >> consistent state in case of errors. And we don't check for errors >> after __build_packet_message() which may result in sending an >> incomplete message to userspace. > > okay > >> Would you send us patches to address this? Thanks. > > Sure thing. Thanks Pablo. > Hi Pablo, While we are at this, what about this one, does it really have to be a BUG() one? __build_packet_message() ... if (data_len) { struct nlattr *nla; int size = nla_attr_size(data_len); if (skb_tailroom(inst->skb) < nla_total_size(data_len)) goto nla_put_failure; <-- already changed nla = (struct nlattr *)skb_put(inst->skb, nla_total_size(data_len)); nla->nla_type = NFULA_PAYLOAD; nla->nla_len = size; if (skb_copy_bits(skb, 0, nla_data(nla), data_len)) BUG(); <-- } Seems we could just put a goto nla_put_failure there too instead of bringing everything down. skb_copy_bits will only fail if we try to copy too much from the skb.. We could leave a WARN_ONCE in there if the idea is to catch nasty bugs in there. WDYT? I'm thinking in just sticking with a goto in there too. Marcelo -- 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
On Tue, Oct 28, 2014 at 05:56:31PM -0200, Marcelo Ricardo Leitner wrote: > Hi Pablo, > > While we are at this, what about this one, does it really have to be a BUG() one? > > __build_packet_message() > ... > if (data_len) { > struct nlattr *nla; > int size = nla_attr_size(data_len); > > if (skb_tailroom(inst->skb) < nla_total_size(data_len)) > goto nla_put_failure; <-- already changed > > nla = (struct nlattr *)skb_put(inst->skb, nla_total_size(data_len)); > nla->nla_type = NFULA_PAYLOAD; > nla->nla_len = size; > > if (skb_copy_bits(skb, 0, nla_data(nla), data_len)) > BUG(); <-- > } > > Seems we could just put a goto nla_put_failure there too instead of > bringing everything down. skb_copy_bits will only fail if we try to > copy too much from the skb.. We could leave a WARN_ONCE in there if > the idea is to catch nasty bugs in there. WDYT? I'm thinking in just > sticking with a goto in there too. I think this is most likely catching a malformed skbuff coming from the driver, in that case we shouldn't go further. There's similar handling in other part of the kernel. This may also trigger the BUG() if data_len is miscalculated as you said, but that seems less likely to happen to me. I would leave that one as it is. -- 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
On 28-10-2014 18:12, Pablo Neira Ayuso wrote: > On Tue, Oct 28, 2014 at 05:56:31PM -0200, Marcelo Ricardo Leitner wrote: >> Hi Pablo, >> >> While we are at this, what about this one, does it really have to be a BUG() one? >> >> __build_packet_message() >> ... >> if (data_len) { >> struct nlattr *nla; >> int size = nla_attr_size(data_len); >> >> if (skb_tailroom(inst->skb) < nla_total_size(data_len)) >> goto nla_put_failure; <-- already changed >> >> nla = (struct nlattr *)skb_put(inst->skb, nla_total_size(data_len)); >> nla->nla_type = NFULA_PAYLOAD; >> nla->nla_len = size; >> >> if (skb_copy_bits(skb, 0, nla_data(nla), data_len)) >> BUG(); <-- >> } >> >> Seems we could just put a goto nla_put_failure there too instead of >> bringing everything down. skb_copy_bits will only fail if we try to >> copy too much from the skb.. We could leave a WARN_ONCE in there if >> the idea is to catch nasty bugs in there. WDYT? I'm thinking in just >> sticking with a goto in there too. > > I think this is most likely catching a malformed skbuff coming from > the driver, in that case we shouldn't go further. There's similar > handling in other part of the kernel. > > This may also trigger the BUG() if data_len is miscalculated as you > said, but that seems less likely to happen to me. > > I would leave that one as it is. Cool, thanks. Marcelo -- 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 --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c index b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..0eb10bf295e92ad29541f58ca1de67e3380157c0 100644 --- a/net/netfilter/nfnetlink_log.c +++ b/net/netfilter/nfnetlink_log.c @@ -12,6 +12,8 @@ * 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> @@ -45,9 +47,6 @@ #define NFULNL_QTHRESH_DEFAULT 100 /* 100 packets */ #define NFULNL_COPY_RANGE_MAX 0xFFFF /* max packet size is limited by 16-bit struct nfattr nfa_len field */ -#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; @@ -335,8 +334,7 @@ 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); + pr_err("can't even alloc %u bytes\n", pkt_size); } } @@ -569,7 +567,7 @@ __build_packet_message(struct nfnl_log_net *log, 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"); + pr_warn("no tailroom!\n"); return -1; } @@ -585,7 +583,7 @@ __build_packet_message(struct nfnl_log_net *log, return 0; nla_put_failure: - PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n"); + net_err_ratelimited("error creating log nlmsg\n"); return -1; } @@ -1068,19 +1066,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;
And also remove PRINTR macro in favor of net_*_ratelimited(). Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com> --- Notes: v1-v2: make use of net_err_ratelimited() net/netfilter/nfnetlink_log.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)