On Tue, Jun 11, 2019 at 08:09:12AM -0400, Stephen Suryaputra wrote:
[...]
> diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
> index a940c9fd9045..4155a32fade7 100644
> --- a/net/netfilter/nft_exthdr.c
> +++ b/net/netfilter/nft_exthdr.c
> @@ -62,6 +62,125 @@ static void nft_exthdr_ipv6_eval(const struct nft_expr *expr,
> regs->verdict.code = NFT_BREAK;
> }
>
> +/* find the offset to specified option or the header beyond the options
> + * if target < 0.
> + *
> + * If target header is found, its offset is set in *offset and return option
> + * number. Otherwise, return negative error.
> + *
> + * If the first fragment doesn't contain the End of Options it is considered
> + * invalid.
> + */
> +static int ipv4_find_option(struct net *net, struct sk_buff *skb,
> + unsigned int *offset, int target,
> + unsigned short *fragoff, int *flags)
flags is never used, please remove it.
> +{
> + unsigned char optbuf[sizeof(struct ip_options) + 41];
In other parts of the kernel this is + 40:
net/ipv4/cipso_ipv4.c: unsigned char optbuf[sizeof(struct ip_options) + 40];
here it is + 41.
> + struct ip_options *opt = (struct ip_options *)optbuf;
> + struct iphdr *iph, _iph;
> + unsigned int start;
> + bool found = false;
> + __be32 info;
> + int optlen;
> +
> + if (fragoff)
> + *fragoff = 0;
fragoff is set and never used. Please, remove this parameter.
> + iph = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> + if (!iph || iph->version != 4)
> + return -EBADMSG;
> + start = sizeof(struct iphdr);
> +
> + optlen = iph->ihl * 4 - (int)sizeof(struct iphdr);
> + if (optlen <= 0)
> + return -ENOENT;
> +
> + memset(opt, 0, sizeof(struct ip_options));
> + /* Copy the options since __ip_options_compile() modifies
> + * the options. Get one byte beyond the option for target < 0
How does this "one byte beyond the option" trick works?
> + */
> + if (skb_copy_bits(skb, start, opt->__data, optlen + 1))
> + return -EBADMSG;
> + opt->optlen = optlen;
> +
> + if (__ip_options_compile(net, opt, NULL, &info))
> + return -EBADMSG;
> +
> + switch (target) {
> + case IPOPT_SSRR:
> + case IPOPT_LSRR:
> + if (!opt->srr)
> + break;
> + found = target == IPOPT_SSRR ? opt->is_strictroute :
> + !opt->is_strictroute;
> + if (found)
> + *offset = opt->srr + start;
> + break;
> + case IPOPT_RR:
> + if (opt->rr)
> + break;
> + *offset = opt->rr + start;
> + found = true;
> + break;
> + case IPOPT_RA:
> + if (opt->router_alert)
> + break;
> + *offset = opt->router_alert + start;
> + found = true;
> + break;
> + default:
> + /* Either not supported or not a specific search, treated as
> + * found
> + */
> + found = true;
> + if (target >= 0) {
> + target = -EOPNOTSUPP;
> + break;
> + }
> + if (opt->end) {
> + *offset = opt->end + start;
> + target = IPOPT_END;
May I ask, what's the purpose of IPOPT_END? :-)
> + } else {
> + /* Point to beyond the options. */
> + *offset = optlen + start;
> + target = opt->__data[optlen];
> + }
> + }
> + if (!found)
> + target = -ENOENT;
> + return target;
nitpick: Probably replace code above.
return found ? target : -ENOENT;
Apart from the above, this looks good to me.
Thanks!
