Re: [PATCH nf-next v2] nft_osf: Add ttl option support

[Pablo Neira Ayuso]

Hi Fernando,

A few comments.

On Sat, Sep 29, 2018 at 12:18:51PM +0200, Fernando Fernandez Mancera wrote:
> Add ttl option support to the nftables "osf" expression.
> 
> Signed-off-by: Fernando Fernandez Mancera <ffmanc...@riseup.net>
> ---
> v1: initial patch
> v2: code correctness and fix the "~" typo.
> ---
>  include/linux/netfilter/nfnetlink_osf.h  |  3 ++-
>  include/uapi/linux/netfilter/nf_tables.h |  7 +++++++
>  net/netfilter/nfnetlink_osf.c            |  5 +++--
>  net/netfilter/nft_osf.c                  | 18 +++++++++++++++++-
>  4 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/netfilter/nfnetlink_osf.h 
> b/include/linux/netfilter/nfnetlink_osf.h
> index ecf7dab81e9e..c6000046c966 100644
> --- a/include/linux/netfilter/nfnetlink_osf.h
> +++ b/include/linux/netfilter/nfnetlink_osf.h
> @@ -27,6 +27,7 @@ bool nf_osf_match(const struct sk_buff *skb, u_int8_t 
> family,
>                 const struct list_head *nf_osf_fingers);
>  
>  const char *nf_osf_find(const struct sk_buff *skb,
> -                        const struct list_head *nf_osf_fingers);
> +                     const struct list_head *nf_osf_fingers,
> +                     const int ttl_check);
>  
>  #endif /* _NFOSF_H */
> diff --git a/include/uapi/linux/netfilter/nf_tables.h 
> b/include/uapi/linux/netfilter/nf_tables.h
> index 5444e76870bb..579974b0bf0d 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -1511,9 +1511,16 @@ enum nft_flowtable_hook_attributes {
>  };
>  #define NFTA_FLOWTABLE_HOOK_MAX      (__NFTA_FLOWTABLE_HOOK_MAX - 1)
>  
> +/**
> + * enum nft_osf_attributes - nftables osf expression netlink attributes
> + *
> + * @NFTA_OSF_DREG: destination register (NLA_U32: nft_registers)
> + * @NFTA_OSF_TTL: Value of the TTL osf option (NLA_U8)
> + */
>  enum nft_osf_attributes {
>       NFTA_OSF_UNSPEC,
>       NFTA_OSF_DREG,
> +     NFTA_OSF_TTL,
>       __NFTA_OSF_MAX,
>  };
>  #define NFTA_OSF_MAX (__NFTA_OSF_MAX - 1)
> diff --git a/net/netfilter/nfnetlink_osf.c b/net/netfilter/nfnetlink_osf.c
> index 00db27dfd2ff..ec8fa9661538 100644
> --- a/net/netfilter/nfnetlink_osf.c
> +++ b/net/netfilter/nfnetlink_osf.c
> @@ -257,7 +257,8 @@ nf_osf_match(const struct sk_buff *skb, u_int8_t family,
>  EXPORT_SYMBOL_GPL(nf_osf_match);
>  
>  const char *nf_osf_find(const struct sk_buff *skb,
> -                     const struct list_head *nf_osf_fingers)
> +                     const struct list_head *nf_osf_fingers,
> +                     const int ttl_check)
>  {
>       const struct iphdr *ip = ip_hdr(skb);
>       const struct nf_osf_user_finger *f;
> @@ -275,7 +276,7 @@ const char *nf_osf_find(const struct sk_buff *skb,
>  
>       list_for_each_entry_rcu(kf, &nf_osf_fingers[ctx.df], finger_entry) {
>               f = &kf->finger;
> -             if (!nf_osf_match_one(skb, f, -1, &ctx))
> +             if (!nf_osf_match_one(skb, f, ttl_check, &ctx))
>                       continue;
>  
>               genre = f->genre;
> diff --git a/net/netfilter/nft_osf.c b/net/netfilter/nft_osf.c
> index 5af74b37f423..0fbaaafff8d8 100644
> --- a/net/netfilter/nft_osf.c
> +++ b/net/netfilter/nft_osf.c
> @@ -6,10 +6,12 @@
>  
>  struct nft_osf {
>       enum nft_registers      dreg:8;
> +     u8                      ttl;
>  };
>  
>  static const struct nla_policy nft_osf_policy[NFTA_OSF_MAX + 1] = {
>       [NFTA_OSF_DREG]         = { .type = NLA_U32 },
> +     [NFTA_OSF_TTL]          = { .type = NLA_U8 },
>  };
>  
>  static void nft_osf_eval(const struct nft_expr *expr, struct nft_regs *regs,
> @@ -21,6 +23,7 @@ static void nft_osf_eval(const struct nft_expr *expr, 
> struct nft_regs *regs,
>       const struct tcphdr *tcp;
>       struct tcphdr _tcph;
>       const char *os_name;
> +     int ttl_check;
>  
>       tcp = skb_header_pointer(skb, ip_hdrlen(skb),
>                                sizeof(struct tcphdr), &_tcph);
> @@ -33,7 +36,8 @@ static void nft_osf_eval(const struct nft_expr *expr, 
> struct nft_regs *regs,
>               return;
>       }
>  
> -     os_name = nf_osf_find(skb, nf_osf_fingers);
> +     ttl_check = priv->ttl;
> +     os_name = nf_osf_find(skb, nf_osf_fingers, ttl_check);

This should be fine, so we don't need the extra 'ttl_check' variable:

        os_name = nf_osf_find(skb, nf_osf_fingers, priv->ttl);

See more comments below.

>       if (!os_name)
>               strncpy((char *)dest, "unknown", NFT_OSF_MAXGENRELEN);
>       else
> @@ -46,6 +50,15 @@ static int nft_osf_init(const struct nft_ctx *ctx,
>  {
>       struct nft_osf *priv = nft_expr_priv(expr);
>       int err;
> +     u8 ttl;
> +
> +     if (!tb[NFTA_OSF_TTL])
> +             return -EINVAL;
> +
> +     ttl = nla_get_u8(tb[NFTA_OSF_TTL]);
> +     if (ttl > 2)
> +             return -EOPNOTSUPP;
> +     priv->ttl = nla_get_u8(tb[NFTA_OSF_TTL]);

Better make this optional, ie.

        if (tb[NFTA_OSF_TTL]) {
                ttl = nla_get_u8(tb[NFTA_OSF_TTL]);
                if (ttl > 2)
                        return -EINVAL;

                priv->ttl = ttl;
        }

What is the good default value for priv->ttl? We were using -1 before
this patch, but now ttl is u8, while nf_osf_ttl() takes a signed
value. Better make this s8 instead of u8?

Thanks.