| Message ID | 20181004115717.620-1-ffmancera@riseup.net |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf-next,v3] nft_osf: Add ttl option support | expand |
On Thu, Oct 04, 2018 at 01:57:17PM +0200, Fernando Fernandez Mancera wrote: [...] > diff --git a/net/netfilter/nfnetlink_osf.c b/net/netfilter/nfnetlink_osf.c > index 00db27dfd2ff..e0fe1b8429ac 100644 > --- a/net/netfilter/nfnetlink_osf.c > +++ b/net/netfilter/nfnetlink_osf.c > @@ -32,9 +32,7 @@ static inline int nf_osf_ttl(const struct sk_buff *skb, > { > const struct iphdr *ip = ip_hdr(skb); > > - if (ttl_check != -1) { > - if (ttl_check == NF_OSF_TTL_TRUE) > - return ip->ttl == f_ttl; > + if (ttl_check != 0) { May ttl_check now ever be -1 now that we do not need it in nft_osf? If xt_osf never does it, we can probably remove this branch, ie. - if (ttl_check != -1) { and save one level of indentation. This would need a careful look at current xt_osf.c - as well as its previous one code - to make sure we do not break anything if we remove this ttl_check != -1 branch.
On 10/4/18 2:03 PM, Pablo Neira Ayuso wrote: > On Thu, Oct 04, 2018 at 01:57:17PM +0200, Fernando Fernandez Mancera wrote: > [...] >> diff --git a/net/netfilter/nfnetlink_osf.c b/net/netfilter/nfnetlink_osf.c >> index 00db27dfd2ff..e0fe1b8429ac 100644 >> --- a/net/netfilter/nfnetlink_osf.c >> +++ b/net/netfilter/nfnetlink_osf.c >> @@ -32,9 +32,7 @@ static inline int nf_osf_ttl(const struct sk_buff *skb, >> { >> const struct iphdr *ip = ip_hdr(skb); >> >> - if (ttl_check != -1) { >> - if (ttl_check == NF_OSF_TTL_TRUE) >> - return ip->ttl == f_ttl; >> + if (ttl_check != 0) { > > May ttl_check now ever be -1 now that we do not need it in nft_osf? > > If xt_osf never does it, we can probably remove this branch, ie. > > - if (ttl_check != -1) { > > and save one level of indentation. > > This would need a careful look at current xt_osf.c - as well as its > previous one code - to make sure we do not break anything if we remove > this ttl_check != -1 branch. > Currently in xt_osf.c if the option "--ttl" is not found by default, after the v3 iteration, it sets ttl_check to 0 as you can see here: @@ -213,7 +211,7 @@ nf_osf_match(const struct sk_buff *skb, u_int8_t family, if (!tcp) return false; - ttl_check = (info->flags & NF_OSF_TTL) ? info->ttl : -1; + ttl_check = (info->flags & NF_OSF_TTL) ? info->ttl : 0; I think we can remove the "ttl_check != -1" branch and keep the if statement that checks "ttl_check == NF_OSF_TTL_TRUE". I can do it and test if it breaks something in xt_osf.c but I think this change should break nothing.
On Thu, Oct 04, 2018 at 02:17:53PM +0200, Fernando Fernandez Mancera wrote: > On 10/4/18 2:03 PM, Pablo Neira Ayuso wrote: > > On Thu, Oct 04, 2018 at 01:57:17PM +0200, Fernando Fernandez Mancera wrote: > > [...] > > > diff --git a/net/netfilter/nfnetlink_osf.c b/net/netfilter/nfnetlink_osf.c > > > index 00db27dfd2ff..e0fe1b8429ac 100644 > > > --- a/net/netfilter/nfnetlink_osf.c > > > +++ b/net/netfilter/nfnetlink_osf.c > > > @@ -32,9 +32,7 @@ static inline int nf_osf_ttl(const struct sk_buff *skb, > > > { > > > const struct iphdr *ip = ip_hdr(skb); > > > - if (ttl_check != -1) { > > > - if (ttl_check == NF_OSF_TTL_TRUE) > > > - return ip->ttl == f_ttl; > > > + if (ttl_check != 0) { > > > > May ttl_check now ever be -1 now that we do not need it in nft_osf? > > > > If xt_osf never does it, we can probably remove this branch, ie. > > > > - if (ttl_check != -1) { > > > > and save one level of indentation. > > > > This would need a careful look at current xt_osf.c - as well as its > > previous one code - to make sure we do not break anything if we remove > > this ttl_check != -1 branch. > > > > Currently in xt_osf.c if the option "--ttl" is not found by default, after > the v3 iteration, it sets ttl_check to 0 as you can see here: > > @@ -213,7 +211,7 @@ nf_osf_match(const struct sk_buff *skb, u_int8_t family, > if (!tcp) > return false; > > - ttl_check = (info->flags & NF_OSF_TTL) ? info->ttl : -1; > + ttl_check = (info->flags & NF_OSF_TTL) ? info->ttl : 0; > > I think we can remove the "ttl_check != -1" branch and keep the if statement > that checks "ttl_check == NF_OSF_TTL_TRUE". I can do it and test if it > breaks something in xt_osf.c but I think this change should break nothing. Then, go ahead remove it. Thanks.
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..e0fe1b8429ac 100644 --- a/net/netfilter/nfnetlink_osf.c +++ b/net/netfilter/nfnetlink_osf.c @@ -32,9 +32,7 @@ static inline int nf_osf_ttl(const struct sk_buff *skb, { const struct iphdr *ip = ip_hdr(skb); - if (ttl_check != -1) { - if (ttl_check == NF_OSF_TTL_TRUE) - return ip->ttl == f_ttl; + if (ttl_check != 0) { if (ttl_check == NF_OSF_TTL_NOCHECK) return 1; else if (ip->ttl <= f_ttl) @@ -213,7 +211,7 @@ nf_osf_match(const struct sk_buff *skb, u_int8_t family, if (!tcp) return false; - ttl_check = (info->flags & NF_OSF_TTL) ? info->ttl : -1; + ttl_check = (info->flags & NF_OSF_TTL) ? info->ttl : 0; list_for_each_entry_rcu(kf, &nf_osf_fingers[ctx.df], finger_entry) { @@ -257,7 +255,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 +274,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..63c7199a441d 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, @@ -33,7 +35,7 @@ static void nft_osf_eval(const struct nft_expr *expr, struct nft_regs *regs, return; } - os_name = nf_osf_find(skb, nf_osf_fingers); + os_name = nf_osf_find(skb, nf_osf_fingers, priv->ttl); if (!os_name) strncpy((char *)dest, "unknown", NFT_OSF_MAXGENRELEN); else @@ -46,6 +48,14 @@ static int nft_osf_init(const struct nft_ctx *ctx, { struct nft_osf *priv = nft_expr_priv(expr); int err; + u8 ttl; + + if (nla_get_u8(tb[NFTA_OSF_TTL])) { + ttl = nla_get_u8(tb[NFTA_OSF_TTL]); + if (ttl > 2) + return -EINVAL; + priv->ttl = ttl; + } priv->dreg = nft_parse_register(tb[NFTA_OSF_DREG]); err = nft_validate_register_store(ctx, priv->dreg, NULL, @@ -60,6 +70,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;
Add ttl option support to the nftables "osf" expression. Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net> --- v1:initial patch v2:v2: code correctness and fix the "~" typo. v3:make priv->ttl = ttl; optional and priv->ttl default value is now 0. --- include/linux/netfilter/nfnetlink_osf.h | 3 ++- include/uapi/linux/netfilter/nf_tables.h | 7 +++++++ net/netfilter/nfnetlink_osf.c | 11 +++++------ net/netfilter/nft_osf.c | 15 ++++++++++++++- 4 files changed, 28 insertions(+), 8 deletions(-)