Re: [PATCH v2 3/3] Make use of pr_fmt where applicable

[Marcelo Ricardo Leitner <mleitner@redhat.com>]

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