Re: [PATCH] nf_tables: Transaction API proposal

[Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>]

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