Re: [PATCH v2 nf-next 2/3] netfilter: nf_tables: extend tracing infrastructure

[Florian Westphal <fw@strlen.de>]

Patrick McHardy <kaber@trash.net> wrote:
> On 26.11, Florian Westphal wrote:
> > +++ b/net/netfilter/nf_tables_trace.c
> > @@ -0,0 +1,253 @@
> > +static int trace_fill_header(struct sk_buff *nlskb, u16 type,
> > +			     const struct sk_buff *skb,
> > +			     int off, unsigned int len)
> > +{
> > +	struct nlattr *nla;
> > +
> > +	if (len == 0)
> > +		return 0;
> > +
> > +	if (skb_tailroom(nlskb) < nla_total_size(len))
> > +		return -1;
> > +
> > +	nla = (struct nlattr *)skb_put(nlskb, nla_total_size(len));
> > +	nla->nla_type = type;
> > +	nla->nla_len = nla_attr_size(len);
> 
> nla_reserve()? That will also take care of zeroing the padding.

Fixed, thanks.

> > +static int nf_trace_fill_ll_header(struct sk_buff *nlskb,
> > +				   const struct sk_buff *skb)
> > +{
> > +	struct vlan_ethhdr veth;
> > +	unsigned int len, off;
> > +
> > +	off = skb_mac_header(skb) - skb->data;
> 
> off is negative or zero, it should use a signed int I think.

Right, fixed.

> > +
> > +	if (skb_copy_bits(skb, off, &veth, ETH_HLEN))
> > +		return -1;
> > +
> > +	veth.h_vlan_proto = skb->vlan_proto;
> > +	veth.h_vlan_TCI = htons(skb_vlan_tag_get(skb));
> > +	veth.h_vlan_encapsulated_proto = skb->protocol;
> > +
> > +	len = min_t(unsigned int,
> > +		    skb->data - skb_mac_header(skb), NFT_TRACETYPE_LL_HSIZE);
> 
> min_t(unsigned int, -off, NFT_TRACETYPE_LL_HSIZE) ?

[..]

> Why the min_t at all if we fail on != ETH_HLEN? I'd simply check that
> it is exactly ETH_HLEN before even starting copying.

Good point, I removed min test completely and changed it to:

    int off;

    BUILD_BUG_ON(NFT_TRACETYPE_LL_HSIZE < ETH_HLEN);

    off = skb_mac_header(skb) - skb->data;
    if (len != -ETH_HLEN)
            return -1;
    ...
> > +
> > +	len = sizeof(veth);
> > +	if (skb_tailroom(nlskb) < nla_total_size(len))
> > +		return -1;
> > +
> > +	return nla_put(nlskb, NFTA_TRACE_LL_HEADER, len, &veth);
> 
> nla_put() already checks the tailroom.

Right, my bad.  Removed.

> > +	len = min_t(unsigned int,
> > +		    skb->data - skb_mac_header(skb), NFT_TRACETYPE_LL_HSIZE);
> > +	off = skb_mac_header(skb) - skb->data;
> 
> Could again avoid the double calculation by using -off in min_t().

Fixed.

> > +static int nf_trace_fill_chain_info(struct sk_buff *nlskb,
> > +				    const struct nft_chain *chain)
> > +{
> > +	if (nla_put_string(nlskb, NFTA_TRACE_TABLE, chain->table->name))
> > +		return -1;
> > +
> > +	return nla_put_string(nlskb, NFTA_TRACE_CHAIN, chain->name);
> 
> I would not move simple stuff that is only used once like this to another
> function. Easier to read if you don't have to jump around in the code.

Ok, open-coded this instead.

Thanks for the review, I'll send a v3 soon.