Patrick McHardy <kaber@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
