Re: [PATCH nf-next v2] nft_osf: Add ttl option support
On Thu, Oct 04, 2018 at 01:05:42PM +0200, Fernando Fernandez Mancera wrote:
[...]
> On 10/3/18 5:36 PM, Pablo Neira Ayuso wrote:
[...]
> > 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;
> > }
> >
>
> Seems fine to me, I am going to do it.
>
> > 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.
> >
>
> About the default value for priv->ttl, I think we can use 0 instead of -1.
> Right now, if priv->ttl is -1 the behaviour established is "-m osf --ttl 0".
> What do you think about that?
Fine with me.
Thanks.
Re: [PATCH nf-next v2] nft_osf: Add ttl option support
On 10/3/18 5:36 PM, Pablo Neira Ayuso wrote:
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.
[..]
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;
}
Seems fine to me, I am going to do it.
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.
About the default value for priv->ttl, I think we can use 0 instead of
-1. Right now, if priv->ttl is -1 the behaviour established is "-m osf
--ttl 0". What do you think about that?
Thanks.
Re: [PATCH nf-next v2] nft_osf: Add ttl option support
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
> ---
> 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..c646c966 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, _osf_fingers[ctx.df], finger_entry) {
> f = >finger;
> - if (!nf_osf_match_one(skb, f, -1, ))
> + if (!nf_osf_match_one(skb, f, ttl_check, ))
> 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 >
[PATCH nf-next v2] nft_osf: Add ttl option support
Add ttl option support to the nftables "osf" expression.
Signed-off-by: Fernando Fernandez Mancera
---
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..c646c966 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, _osf_fingers[ctx.df], finger_entry) {
f = >finger;
- if (!nf_osf_match_one(skb, f, -1, ))
+ if (!nf_osf_match_one(skb, f, ttl_check, ))
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);
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]);
priv->dreg = nft_parse_register(tb[NFTA_OSF_DREG]);
err = nft_validate_register_store(ctx, priv->dreg, NULL,
@@ -60,6 +73,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.19.0
