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

[Fernando Fernandez Mancera <ffmancera@xxxxxxxxxx>]

On 9/18/18 1:15 AM, Pablo Neira Ayuso wrote:
> On Sun, Sep 16, 2018 at 09:10:16PM +0200, Fernando Fernandez Mancera wrote:
> > Add ttl option support to the nftables "osf" expression.
> >
> > Signed-off-by: Fernando Fernandez Mancera <ffmancera@xxxxxxxxxx>
> > ---
> >   include/linux/netfilter/nfnetlink_osf.h      |  3 ++-
> >   include/uapi/linux/netfilter/nf_tables.h     |  7 +++++++
> >   include/uapi/linux/netfilter/nfnetlink_osf.h |  1 +
> >   net/netfilter/nfnetlink_osf.c                |  5 +++--
> >   net/netfilter/nft_osf.c                      | 14 +++++++++++++-
> >   5 files changed, 26 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 e23290ffdc77..85df36521153 100644
> > --- a/include/uapi/linux/netfilter/nf_tables.h
> > +++ b/include/uapi/linux/netfilter/nf_tables.h
> > @@ -1493,9 +1493,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/include/uapi/linux/netfilter/nfnetlink_osf.h b/include/uapi/linux/netfilter/nfnetlink_osf.h
> > index 272bc3195f2d..7e0c00658bf8 100644
> > --- a/include/uapi/linux/netfilter/nfnetlink_osf.h
> > +++ b/include/uapi/linux/netfilter/nfnetlink_osf.h
> > @@ -26,6 +26,7 @@
> >   
> >   #define NF_OSF_FLAGMASK		(NF_OSF_GENRE | NF_OSF_TTL | \
> >   				 NF_OSF_LOG | NF_OSF_INVERT)
> > +
>
> You can remove this entire patch chunk. This newline is not needed >
> >   /* Wildcard MSS (kind of).
> >    * It is used to implement a state machine for the different wildcard values
> >    * of the MSS and window sizes.
> > 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..b63c5f5c716b 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);
> >   	if (!os_name)
> >   		strncpy((char *)dest, "unknown", NFT_OSF_MAXGENRELEN);
> >   	else
> > @@ -47,6 +51,11 @@ static int nft_osf_init(const struct nft_ctx *ctx,
> >   	struct nft_osf *priv = nft_expr_priv(expr);
> >   	int err;
> >   
> > +	if (!tb[NFTA_OSF_TTL] || nla_get_u8(tb[NFTA_OSF_TTL]) < 0 ||
> > +	    nla_get_u8(tb[NFTA_OSF_TTL]) > 2)
>
> Better:
>
>          u8 ttl;
>
> 	if (!tb[NFTA_OSF_TTL]) {
>                  ttl = nla_get_u8(tb[NFTA_OSF_TTL]);
>                  if (ttl > 2)
>                          return -EOPNOTSUPP;
>          }
>
> > +		return ~EINVAL;
>                         ^
>                  This is not a minus :-).

Sorry, it is a typo. I am going to fix it.

> > +	priv->ttl = nla_get_u8(tb[NFTA_OSF_TTL]);
> > +
> >   	priv->dreg = nft_parse_register(tb[NFTA_OSF_DREG]);
> >   	err = nft_validate_register_store(ctx, priv->dreg, NULL,
> >   					  NFTA_DATA_VALUE, NFT_OSF_MAXGENRELEN);
> > @@ -60,6 +69,9 @@ static int nft_osf_dump(struct sk_buff *skb, const struct nft_expr *expr)
> >   {
> >   	const struct nft_osf *priv = nft_expr_priv(expr);
> >   
> > +	if (nla_put_u8(skb, NFTA_OSF_TTL, priv->ttl))
> > +		goto nla_put_failure;
> > +
> >   	if (nft_dump_register(skb, NFTA_OSF_DREG, priv->dreg))
> >   		goto nla_put_failure;
> >   
> > --
> > 2.18.0