Re: [PATCH nf-next] netfilter: add support for matching IPv4 options

[Stephen Suryaputra <ssuryaextr@gmail.com>]

On Fri, May 31, 2019 at 07:11:01PM +0200, Pablo Neira Ayuso wrote:
> > +/* find the offset to specified option or the header beyond the options
> > + * if target < 0.
> > + *
> > + * Note that *offset is used as input/output parameter, and if it is not zero,
> > + * then it must be a valid offset to an inner IPv4 header. This can be used
> > + * to explore inner IPv4 header, eg. ICMP error messages.
> 
> In other extension headers (IPv6 and TCP options) this offset is used
> to match for a field inside the extension / option.
> 
> So this semantics you describe here are slightly different, right?

It is the same as the IPv6 one. The offset returned is the offset to the
specific option (target) or the byte beyond the options if the target
isn't specified (< 0).

> Align parameters to parens when breaking too long lines.
> Please, also break lines at 80 chars.
> Please, define variables using reverse xmas tree, ie.

OK on these.

> > +	optlen = iph->ihl * 4 - (int)sizeof(struct iphdr);
> > +	if (optlen <= 0)
> > +		return -ENOENT;
> 
> You could just:
> 
>                 return -1;
> 
> in all these errors in ipv4_find_option() since nft_exthdr_ipv4_eval()
> does not use it.

Yes, but I followed the pattern in ipv6_find_hdr().

> I'd suggest:
> 
>                 if (!opt->srr)
>                         break;
> 
> So you save one level of indentation below.
> 
> same here:
> 
>                 if (!opt->rr)
>                         break;
> 
> and same thing for other extensions.

OK on these.
> 
> > +	if (!found)
> > +		target = -ENOENT;
> > +	return target;
> 
> Hm, 'target' value is never used, right?

Again, this follows ipv6_find_hdr().

> This should also work for the NFPROTO_INET (inet tables), NFPROTO_BRIDGE
> and the NFPROTO_NETDEV families.
> 
> I would turn this into:
> 
> 		if (ctx->family != NFPROTO_IPV6) {
> 

OK.

> > +			if (tb[NFTA_EXTHDR_DREG])
> > +				return &nft_exthdr_ipv4_ops;
> > +		}
> > +		break;
> 
> Then, from the _eval() path:
> 
> You have to replace iph->version == 4 check abive, you could use
> skb->protocol instead, and check for htons(ETH_P_IP) packets.

A bit lost. Did you mean this?

static int ipv4_find_option(struct net *net, struct sk_buff *skb,
»       »       »           unsigned int *offset, int target,
»       »       »           unsigned short *fragoff, int *flags)
{
	...
»       iph = skb_header_pointer(skb, *offset, sizeof(_iph), &_iph);
»       if (!iph || skb->protocol != htons(ETH_P_IP))
»       »       return -EBADMSG;

Thank you.