I have some notes, comments below. Thanks! On 3/6/19 5:55 PM, Fernando Fernandez Mancera wrote: > Sorry about the wrong mailing list, it was a mistake. I am fine with > your comments so I am going to send a v2. Thanks. > > On 3/6/19 5:15 PM, Pablo Neira Ayuso wrote: >> Ups, wrong mailing list. For patches use >> netfilter-devel@xxxxxxxxxxxxxxx, so patchwork catches these patches: >> >> http://patchwork.ozlabs.org/project/netfilter-devel/list/ >> >> Anyway, no worries, comments below. >> >> On Wed, Mar 06, 2019 at 01:44:35PM +0100, Fernando Fernandez Mancera wrote: >>> Add version option support to the nftables "osf" expression. >>> >>> Signed-off-by: Fernando Fernandez Mancera <ffmancera@xxxxxxxxxx> >>> --- >>> include/linux/netfilter/nfnetlink_osf.h | 11 ++++++--- >>> include/uapi/linux/netfilter/nf_tables.h | 6 +++++ >>> net/netfilter/nfnetlink_osf.c | 14 +++++------ >>> net/netfilter/nft_osf.c | 31 ++++++++++++++++++++---- >>> 4 files changed, 47 insertions(+), 15 deletions(-) >>> >>> diff --git a/include/linux/netfilter/nfnetlink_osf.h b/include/linux/netfilter/nfnetlink_osf.h >>> index c6000046c966..788613f36935 100644 >>> --- a/include/linux/netfilter/nfnetlink_osf.h >>> +++ b/include/linux/netfilter/nfnetlink_osf.h >>> @@ -21,13 +21,18 @@ struct nf_osf_finger { >>> struct nf_osf_user_finger finger; >>> }; >>> [...] >>> >>> #endif /* _NFOSF_H */ >>> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h >>> index a66c8de006cc..39bbb79ef4c2 100644 >>> --- a/include/uapi/linux/netfilter/nf_tables.h >>> +++ b/include/uapi/linux/netfilter/nf_tables.h >>> @@ -1522,15 +1522,21 @@ enum nft_flowtable_hook_attributes { >>> * >>> * @NFTA_OSF_DREG: destination register (NLA_U32: nft_registers) >>> * @NFTA_OSF_TTL: Value of the TTL osf option (NLA_U8) >>> + * @NFTA_OSF_FLAGS: flags (NLA_U8) >> >> Please, use NLA_U32 for this. >> Do you mean all osf "flags" attribute right? So I am going to do the proper changes in nft, libnftnl and nf-next. [...] >>> @@ -47,6 +57,7 @@ static int nft_osf_init(const struct nft_ctx *ctx, >>> const struct nlattr * const tb[]) >>> { >>> struct nft_osf *priv = nft_expr_priv(expr); >>> + u8 flags; >>> int err; >>> u8 ttl; >>> >>> @@ -57,6 +68,13 @@ static int nft_osf_init(const struct nft_ctx *ctx, >>> priv->ttl = ttl; >>> } >>> >>> + if (tb[NFTA_OSF_FLAGS]) { >>> + flags = nla_get_u8(tb[NFTA_OSF_FLAGS]); >> >> If you turn this into a NLA_U32, then you have to use here. >> >> flags = ntohl(nla_get_be32(tb[NFTA_OSF_FLAGS])); >> >>> + if (flags != NFT_OSF_F_VERSION && flags != 0) >> >> No need to restrict flags != 0, remove that part of the check. >> I think we need this restriction because if we do not set "version" option the "flags" value is 0. If we remove that part of the check we are forcing to use "version" option always. >>> + return -EINVAL; >>> + priv->flags = flags; >>> + } >>> + >>> priv->dreg = nft_parse_register(tb[NFTA_OSF_DREG]); >>> err = nft_validate_register_store(ctx, priv->dreg, NULL, >>> NFT_DATA_VALUE, NFT_OSF_MAXGENRELEN); >>> @@ -73,6 +91,9 @@ static int nft_osf_dump(struct sk_buff *skb, const struct nft_expr *expr) >>> if (nla_put_u8(skb, NFTA_OSF_TTL, priv->ttl)) >>> goto nla_put_failure; >>> >>> + if (nla_put_u8(skb, NFTA_OSF_FLAGS, priv->flags)) >> >> And >> if (nla_put_be32(skb, NFTA_OSF_FLAGS, ntohl(priv->flags)) >> >> here. >> >> Other than that, patch looks good! >> >>> + goto nla_put_failure; >>> + >>> if (nft_dump_register(skb, NFTA_OSF_DREG, priv->dreg)) >>> goto nla_put_failure; >>> >>> -- >>> 2.20.1 >>>
