Re: [PATCH v2 nf-next 5/5] netfilter: nft: rt nexthop for inet family

["Anders K. Pedersen | Cohaesio" <akp@cohaesio.com>]

Hi Liping,

On thu, 2016-10-20 at 17:13 +0800, Liping Zhang wrote:
> Hi Anders,
> 2016-10-20 2:41 GMT+08:00 Anders K. Pedersen | Cohaesio <akp@cohaesio
> .com>:
> 
> > 
> > +static void nft_rt_inet_get_eval(const struct nft_expr *expr,
> > +                                struct nft_regs *regs,
> > +                                const struct nft_pktinfo *pkt)
> > +{
> > +       const struct nft_rt *priv = nft_expr_priv(expr);
> > +       const struct sk_buff *skb = pkt->skb;
> > +       u32 *dest = &regs->data[priv->dreg];
> > +
> > +       if (unlikely(pkt->pf != priv->family)) {
> > +               WARN_ONCE(1, "Address families do not match\n");
> 
> I think this WARN_ONCE seems unnecessary, in inet family, it's
> possible that
> the family is mismatched.

Well, the suggested userspace implementation doesn't allow a rule that
would hit this warning. It is required to use 'ether type ip ... rt ip
nexthop' or 'ether type ip6 ... rt ip6 nexthop', when nexthop is used
from the inet table.

So I found it reasonable to generate a warning if such a rule somehow
got configured anyway, but if you still feel that it should be removed,
I can do that.

> +               goto err;
> > +       }
> > +
> > +       switch (pkt->pf) {
> > +       case NFPROTO_IPV4:
> > +               switch (priv->key) {
> > +               case NFT_RT_NEXTHOP: {
> > +                       const struct rtable *rt = skb_rtable(skb);
> > +
> > +                       if (!rt)
> > +                               goto err;
> > +                       *dest = rt_nexthop(rt, ip_hdr(skb)->daddr);
> > +                       break;
> > +               }
> > +               default:
> > +                       goto out;
> > +               }
> > +               break;
> > +       case NFPROTO_IPV6:
> > +               switch (priv->key) {
> > +               case NFT_RT_NEXTHOP: {
> > +                       struct rt6_info *rt =
> > +                                       (struct rt6_info
> > *)skb_dst(skb);
> > +
> > +                       if (!rt)
> > +                               goto err;
> > +                       memcpy(dest, rt6_nexthop(rt,
> > &ipv6_hdr(skb)->daddr),
> > +                              sizeof(struct in6_addr));
> > +                       break;
> > +               }
> > +               default:
> > +                       goto out;
> > +               }
> > +               break;
> > +       }
> > +
> 
> This code looks not clean, I think it can be converted to such:
> 
> switch (priv->key) {
>         case NFT_RT_NEXTHOP:
>                 switch (pkt->pf)
>                 ...
>         default:
>                 return nft_rt_get_eval(expr, regs, pkt);
> }

My thinking was that for future rt sub-expressions, it will probably
make sense to move 

		const struct rtable *rt = skb_rtable(skb);

		if (!rt)
			goto err;

outside the inner switch statement, so it doesn't need to be duplicated
for each sub-expression. And ditto for the IPV6 part.

> > 
> > +       return;
> > +out:
> > +       return nft_rt_get_eval(expr, regs, pkt);
> > +err:
> > +       regs->verdict.code = NFT_BREAK;
> > +}
> > +
> > +static int nft_rt_inet_get_init(const struct nft_ctx *ctx,
> > +                               const struct nft_expr *expr,
> > +                               const struct nlattr * const tb[])
> > +{
> > +       struct nft_rt *priv = nft_expr_priv(expr);
> > +       unsigned int len;
> > +
> > +       if (tb[NFTA_RT_KEY] == NULL ||
> > +           tb[NFTA_RT_DREG] == NULL ||
> > +           tb[NFTA_RT_FAMILY] == NULL)
> > +               return -EINVAL;
> > +
> > +       priv->key = ntohl(nla_get_be32(tb[NFTA_RT_KEY]));
> > +       priv->family = ntohl(nla_get_be32(tb[NFTA_RT_FAMILY]));
> > +       switch (priv->key) {
> > +       case NFT_RT_NEXTHOP:
> > +               switch (priv->family) {
> > +               case NFPROTO_IPV4:
> > +                       len = sizeof(u32);
> > +                       break;
> > +               case NFPROTO_IPV6:
> > +                       len = sizeof(struct in6_addr);
> > +                       break;
> > +               }
> > +               len = sizeof(u32);
> > +               break;
> 
> If the family is invalid, you should report -EINVAL to the userspace.
> And the code "len = sizeof(u32);" here makes no sense.

Yes, I'll fix that. Thanks for reviewing.

Regards,
Anders K. Pedersen