On Mon, Mar 11, 2024 at 10:14:54PM +0800, Quan Tian wrote:
> The NFTA_TABLE_USERDATA attribute was ignored on updates. The patch adds
> handling for it to support table comment updates.
dump path is lockless:
if (table->udata) {
if (nla_put(skb, NFTA_TABLE_USERDATA, table->udlen, table->udata))
goto nla_put_failure;
}
there are two things to update at the same time here, table->udata and
table->udlen.
This needs to be reworked fully if updates are required.
nft_userdata {
const u8 *data;
u16 len;
}
then, update struct nft_table to have:
struct nft_userdata __rcu *user;
then, from dump path, rcu_dereference struct nft_userdata pointer.
BTW, does swap() ensure rcu semantics?
> Signed-off-by: Quan Tian <tianquan23@xxxxxxxxx>
> ---
> v2: Change to store userdata in struct nlattr * to ensure atomical update
> v3: Do not call nft_trans_destroy() on table updates in nf_tables_commit()
>
> include/net/netfilter/nf_tables.h | 3 ++
> net/netfilter/nf_tables_api.c | 56 ++++++++++++++++++++++---------
> 2 files changed, 43 insertions(+), 16 deletions(-)
>
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 144dc469ebf8..43c747bd3f80 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -1684,10 +1684,13 @@ struct nft_trans_chain {
>
> struct nft_trans_table {
> bool update;
> + struct nlattr *udata;
> };
>
> #define nft_trans_table_update(trans) \
> (((struct nft_trans_table *)trans->data)->update)
> +#define nft_trans_table_udata(trans) \
> + (((struct nft_trans_table *)trans->data)->udata)
>
> struct nft_trans_elem {
> struct nft_set *set;
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 85088297dd0d..62a2a1798052 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -188,6 +188,17 @@ static struct nlattr *nft_userdata_dup(const struct nlattr *udata, gfp_t gfp)
> return kmemdup(udata, nla_total_size(nla_len(udata)), gfp);
> }
>
> +static bool nft_userdata_is_same(const struct nlattr *a, const struct nlattr *b)
> +{
> + if (!a && !b)
> + return true;
> + if (!a || !b)
> + return false;
> + if (nla_len(a) != nla_len(b))
> + return false;
> + return !memcmp(nla_data(a), nla_data(b), nla_len(a));
> +}
> +
> static void __nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set,
> bool bind)
> {
> @@ -1210,16 +1221,16 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
> {
> struct nft_trans *trans;
> u32 flags;
> + const struct nlattr *udata = ctx->nla[NFTA_TABLE_USERDATA];
> int ret;
>
> - if (!ctx->nla[NFTA_TABLE_FLAGS])
> - return 0;
> -
> - flags = ntohl(nla_get_be32(ctx->nla[NFTA_TABLE_FLAGS]));
> - if (flags & ~NFT_TABLE_F_MASK)
> - return -EOPNOTSUPP;
> + if (ctx->nla[NFTA_TABLE_FLAGS]) {
> + flags = ntohl(nla_get_be32(ctx->nla[NFTA_TABLE_FLAGS]));
> + if (flags & ~NFT_TABLE_F_MASK)
> + return -EOPNOTSUPP;
> + }
>
> - if (flags == ctx->table->flags)
> + if (flags == ctx->table->flags && nft_userdata_is_same(udata, ctx->table->udata))
> return 0;
>
> if ((nft_table_has_owner(ctx->table) &&
> @@ -1240,6 +1251,14 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
> if (trans == NULL)
> return -ENOMEM;
>
> + if (udata) {
> + nft_trans_table_udata(trans) = nft_userdata_dup(udata, GFP_KERNEL_ACCOUNT);
> + if (!nft_trans_table_udata(trans)) {
> + ret = -ENOMEM;
> + goto err_table_udata;
> + }
> + }
> +
> if ((flags & NFT_TABLE_F_DORMANT) &&
> !(ctx->table->flags & NFT_TABLE_F_DORMANT)) {
> ctx->table->flags |= NFT_TABLE_F_DORMANT;
> @@ -1271,6 +1290,8 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
>
> err_register_hooks:
> ctx->table->flags |= NFT_TABLE_F_DORMANT;
> + kfree(nft_trans_table_udata(trans));
> +err_table_udata:
> nft_trans_destroy(trans);
> return ret;
> }
> @@ -9378,6 +9399,9 @@ static void nft_obj_commit_update(struct nft_trans *trans)
> static void nft_commit_release(struct nft_trans *trans)
> {
> switch (trans->msg_type) {
> + case NFT_MSG_NEWTABLE:
> + kfree(nft_trans_table_udata(trans));
> + break;
> case NFT_MSG_DELTABLE:
> case NFT_MSG_DESTROYTABLE:
> nf_tables_table_destroy(&trans->ctx);
> @@ -10140,19 +10164,18 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
> switch (trans->msg_type) {
> case NFT_MSG_NEWTABLE:
> if (nft_trans_table_update(trans)) {
> - if (!(trans->ctx.table->flags & __NFT_TABLE_F_UPDATE)) {
> - nft_trans_destroy(trans);
> - break;
> + if (trans->ctx.table->flags & __NFT_TABLE_F_UPDATE) {
> + if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
> + nf_tables_table_disable(net, trans->ctx.table);
> + trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
> }
> - if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
> - nf_tables_table_disable(net, trans->ctx.table);
> -
> - trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
> + swap(trans->ctx.table->udata, nft_trans_table_udata(trans));
> + nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
> } else {
> nft_clear(net, trans->ctx.table);
> + nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
> + nft_trans_destroy(trans);
> }
> - nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
> - nft_trans_destroy(trans);
> break;
> case NFT_MSG_DELTABLE:
> case NFT_MSG_DESTROYTABLE:
> @@ -10430,6 +10453,7 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
> switch (trans->msg_type) {
> case NFT_MSG_NEWTABLE:
> if (nft_trans_table_update(trans)) {
> + kfree(nft_trans_table_udata(trans));
> if (!(trans->ctx.table->flags & __NFT_TABLE_F_UPDATE)) {
> nft_trans_destroy(trans);
> break;
> --
> 2.39.3 (Apple Git-145)
>
