Re: [PATCH 2/4 v6] libnftnl: rule: Change the "userdata" attribute to use new TLV buffer

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

 



On 14/04/16 01:59, Pablo Neira Ayuso wrote:
On Tue, Mar 22, 2016 at 08:46:25PM +0100, Carlos Falgueras García wrote:
diff --git a/src/rule.c b/src/rule.c
index 3a32bf6..db96e5b 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -28,6 +28,7 @@
  #include <libnftnl/rule.h>
  #include <libnftnl/set.h>
  #include <libnftnl/expr.h>
+#include <libnftnl/udata.h>

  struct nftnl_rule {
  	struct list_head head;
@@ -38,10 +39,7 @@ struct nftnl_rule {
  	const char	*chain;
  	uint64_t	handle;
  	uint64_t	position;
-	struct {
-			void		*data;
-			uint32_t	len;
-	} user;
+	struct nftnl_udata_buf	*userdata;

This change, we don't need it, see below.

@@ -162,8 +171,14 @@ void nftnl_rule_set_data(struct nftnl_rule *r, uint16_t attr,
  		r->position = *((uint64_t *)data);
  		break;
  	case NFTNL_RULE_USERDATA:
-		r->user.data = (void *)data;
-		r->user.len = data_len;
+		if (r->flags & (1 << NFTNL_RULE_USERDATA))
+			nftnl_udata_buf_free(r->userdata);
+		r->userdata = nftnl_udata_buf_alloc(data_len);
+		if (!r->userdata) {
+			perror("nftnl_rule_set_data - userdata");
+			return;
+		}
+		nftnl_udata_buf_put(r->userdata, data, data_len);

Think about this: From nft, we allocate the udata_buf, then we add the
attributes. Once we have the TLV area ready, we pass it as a pointer
via nftnl_rule_set_data().

Then, from libnftnl you allocate nftnl_udata_buf_alloc() again, but we
actually don't need this. The reason is that udata_buf is *only*
needed to build the sequence of TLVs. Once we're done with it, we can
just pass the memory blob area that contains this info.

I don't understand. With the modification to the patch [4/4 v6] you made, there is still a data copy and it is passed in the same way. This copy has moved from libnftnl to nft, but it still exists. Diff between the original and modified version of "netlink_linearize_rule" function:

--- carlos/src/netlink_linearize.c
+++ pablo/src/netlink_linearize.c
@@ -1163,7 +1163,6 @@ void netlink_linearize_rule(struct netlink_ctx *ctx, struct nftnl_rule *nlr,
 			    const struct rule *rule)
 {
 	struct netlink_linearize_ctx lctx;
-	struct nftnl_udata_buf *udata;
 	const struct stmt *stmt;

 	memset(&lctx, 0, sizeof(lctx));
@@ -1174,6 +1173,10 @@ void netlink_linearize_rule(struct netlink_ctx *ctx, struct nftnl_rule *nlr,
 		netlink_gen_stmt(&lctx, stmt);

 	if (rule->comment) {
+		struct nftnl_udata_buf *udata;
+		uint32_t udlen;
+		void *ud;
+
 		udata = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
 		if (!udata)
 			memory_allocation_error();
@@ -1182,9 +1185,11 @@ void netlink_linearize_rule(struct netlink_ctx *ctx, struct nftnl_rule *nlr,
 					  rule->comment))
 			memory_allocation_error();

-		nftnl_rule_set_data(nlr, NFTNL_RULE_USERDATA,
-				    nftnl_udata_buf_data(udata),
-				    nftnl_udata_buf_len(udata));
+		udlen = nftnl_udata_buf_len(udata);
+		ud = xmalloc(udlen);
+		memcpy(ud, nftnl_udata_buf_data(udata), udlen);
+
+		nftnl_rule_set_data(nlr, NFTNL_RULE_USERDATA, ud, udlen);

 		nftnl_udata_buf_free(udata);
 	}


For this reason, I'm entirely keeping back this patch.

Good thing though is that we don't need it to get this working :-)


Now there are a couple of leaks because this patch added a couple of neccessary 'frees':

@@ -75,6 +82,8 @@ void nftnl_rule_free(struct nftnl_rule *r)
 		xfree(r->table);
 	if (r->chain != NULL)
 		xfree(r->chain);
+	if (r->flags & (1 << NFTNL_RULE_USERDATA))
+		nftnl_udata_buf_free(r->userdata);

 	xfree(r);
 }

and

@@ -162,8 +171,14 @@ void nftnl_rule_set_data(struct nftnl_rule *r, uint16_t attr,
 		r->position = *((uint64_t *)data);
 		break;
 	case NFTNL_RULE_USERDATA:
-		r->user.data = (void *)data;
-		r->user.len = data_len;
+		if (r->flags & (1 << NFTNL_RULE_USERDATA))
+			nftnl_udata_buf_free(r->userdata);
+		r->userdata = nftnl_udata_buf_alloc(data_len);
+		if (!r->userdata) {
+			perror("nftnl_rule_set_data - userdata");
+			return;
+		}
+		nftnl_udata_buf_put(r->userdata, data, data_len);
 		break;
 	}

There are another 'frees' I think is better to change it for 'nftnl_udata_buf_free', although in practice it is the same.

If you agree, I'll send a modified version of patch [2/4 v6] without the xml and json chunks, but keeping "struct nftnl_udata_buf" in "struct nftnl_rule":

 struct nftnl_rule {
 	struct list_head head;
@@ -38,10 +39,7 @@ struct nftnl_rule {
 	const char	*chain;
 	uint64_t	handle;
 	uint64_t	position;
-	struct {
-			void		*data;
-			uint32_t	len;
-	} user;
+	struct nftnl_udata_buf	*userdata;
 	struct {
 			uint32_t	flags;
 			uint32_t	proto;
--
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