Re: [PATCH] nf_tables: Transaction API proposal

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Pablo,

--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -37,8 +37,9 @@ enum nf_tables_msg_types {
  	NFT_MSG_NEWSETELEM,
  	NFT_MSG_GETSETELEM,
  	NFT_MSG_DELSETELEM,
-	NFT_MSG_COMMIT,
-	NFT_MSG_ABORT,
+	NFT_MSG_START_TRANSACTION,
+	NFT_MSG_COMMIT_TRANSACTION,
+	NFT_MSG_ABORT_TRANSACTION,
No need to rename this and use long names, I would leave them as:

NFT_MSG_BEGIN
NFT_MSG_COMMIT
NFT_MSG_ABORT

I did that change to get a fully explicit message name, as all the other ones are actually.
Why not shortening to NFT_MSG_BEGIN_TRANS then? or something like that.


  	NFT_MSG_MAX,
  };
@@ -88,18 +89,12 @@ enum nft_chain_attributes {
  };
  #define NFTA_CHAIN_MAX		(__NFTA_CHAIN_MAX - 1)
-enum {
-	NFT_RULE_F_COMMIT	= (1 << 0),
-	NFT_RULE_F_MASK		= NFT_RULE_F_COMMIT,
-};
I like the idea of removing the COMMIT flag by the explicit
NFT_MSG_BEGIN.

-static int nf_tables_dirty_add(struct nft_rule *rule, const struct nft_ctx *ctx)
+static int nf_tables_transaction_add(const struct nft_ctx *ctx,
+				     struct nft_transaction *transaction,
+				     struct nft_rule *rule)
  {
  	struct nft_rule_update *rupd;
- /* Another socket owns the dirty list? */
-	if (!ctx->net->nft.pid_owner)
-		ctx->net->nft.pid_owner = ctx->nlh->nlmsg_pid;
-	else if (ctx->net->nft.pid_owner != ctx->nlh->nlmsg_pid)
-		return -EBUSY;
We still need that there is a single owner at time. Otherwise two
ongoing transactions may overlap.

One of the point of this RFC is to propose a way to enable transaction per-client. It's actually not nice to enable only one transaction at a time, what do we do if the owner never commits?
That's why I thought I could store client's transaction somewhere.

But my proposal is bogus anyway as you noticed below, about sk_user_data.


+static int nf_tables_start_transaction(struct sock *nlsk, struct sk_buff *skb,
+				       const struct nlmsghdr *nlh,
+				       const struct nlattr * const nla[])
+{
+	struct nft_transaction *transaction;
+
+	if (nlsk->sk_user_data != NULL)
+		return -EALREADY;
+
+	transaction = kmalloc(sizeof(struct nft_transaction), GFP_KERNEL);
+	if (transaction == NULL)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&transaction->updates);
+	nlsk->sk_user_data = transaction;
This is global to all other subsystems sharing the nfnetlink bus. This
patch will be smaller if you reuse the existing per-net list that is
used for rule updates.

Ok I was suspecting something like that about this socket. I first thought it was tight to the client. We have to figure out something else then, having a list of pid_owner + transaction list.
We could also limit this list to very few amount of owners, let's say 5?

Of course this would lead to lookup in this list every time a request is made, to know whether or not the pid_owner has started a transaction or not.

I will prepare another RFC

Tomasz
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Netfitler Users]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux