[PATCH nf] netfilter: nft_ct: fix unpaired nf_connlabels_get/put call

[PATCH nf] netfilter: nft_ct: fix unpaired nf_connlabels_get/put call

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

We only get nf_connlabels if the user add ct label set expr successfully,
but we will also put nf_connlabels if the user delete ct lable get expr.
This is mismathced, and will cause ct label expr cannot work properly.

Also, if we init something fail, we should put nf_connlabels back.
Otherwise, we may waste to alloc the memory that will never be used.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/nft_ct.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index 81fbb45..4726a2e 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -359,6 +359,7 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
 			   const struct nlattr * const tb[])
 {
 	struct nft_ct *priv = nft_expr_priv(expr);
+	bool label_got = false;
 	unsigned int len;
 	int err;
 
@@ -377,6 +378,7 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
 		err = nf_connlabels_get(ctx->net, (len * BITS_PER_BYTE) - 1);
 		if (err)
 			return err;
+		label_got = true;
 		break;
 #endif
 	default:
@@ -386,17 +388,28 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
 	priv->sreg = nft_parse_register(tb[NFTA_CT_SREG]);
 	err = nft_validate_register_load(priv->sreg, len);
 	if (err < 0)
-		return err;
+		goto err1;
 
 	err = nft_ct_l3proto_try_module_get(ctx->afi->family);
 	if (err < 0)
-		return err;
+		goto err1;
 
 	return 0;
+
+err1:
+	if (label_got)
+		nf_connlabels_put(ctx->net);
+	return err;
+}
+
+static void nft_ct_get_destroy(const struct nft_ctx *ctx,
+			       const struct nft_expr *expr)
+{
+	nft_ct_l3proto_module_put(ctx->afi->family);
 }
 
-static void nft_ct_destroy(const struct nft_ctx *ctx,
-			   const struct nft_expr *expr)
+static void nft_ct_set_destroy(const struct nft_ctx *ctx,
+			       const struct nft_expr *expr)
 {
 	struct nft_ct *priv = nft_expr_priv(expr);
 
@@ -468,7 +481,7 @@ static const struct nft_expr_ops nft_ct_get_ops = {
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_ct)),
 	.eval		= nft_ct_get_eval,
 	.init		= nft_ct_get_init,
-	.destroy	= nft_ct_destroy,
+	.destroy	= nft_ct_get_destroy,
 	.dump		= nft_ct_get_dump,
 };
 
@@ -477,7 +490,7 @@ static const struct nft_expr_ops nft_ct_set_ops = {
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_ct)),
 	.eval		= nft_ct_set_eval,
 	.init		= nft_ct_set_init,
-	.destroy	= nft_ct_destroy,
+	.destroy	= nft_ct_set_destroy,
 	.dump		= nft_ct_set_dump,
 };
 
-- 
2.5.5

Re: [PATCH nf] netfilter: nft_ct: fix unpaired nf_connlabels_get/put call

[Florian Westphal]

Liping Zhang <zlpnobody@163.com> wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> We only get nf_connlabels if the user add ct label set expr successfully,
> but we will also put nf_connlabels if the user delete ct lable get expr.
> This is mismathced, and will cause ct label expr cannot work properly.
> 
> Also, if we init something fail, we should put nf_connlabels back.
> Otherwise, we may waste to alloc the memory that will never be used.

Acked-by: Florian Westphal <fw@strlen.de>

Unrelated to your patch:

I think its time to change conntrack labels to a pure 128 bit field:

#define NF_CT_LABELS_MAX_SIZE ((XT_CONNLABEL_MAXBIT + 1) / BITS_PER_BYTE)

struct nf_conn_labels {
	unsigned long bits[NF_CT_LABELS_MAX_SIZE];
};

static inline struct nf_conn_labels *nf_ct_labels_ext_add(struct nf_conn *ct)
{
#ifdef CONFIG_NF_CONNTRACK_LABELS
        struct nf_conn_labels *cl_ext;
        struct net *net = nf_ct_net(ct);

        if (net->ct.labels_used == 0)
                return NULL;

        cl_ext = nf_ct_ext_add_length(ct, NF_CT_EXT_LABELS,
                                      sizeof(struct nf_conn_labels), GFP_ATOMIC);
        if (cl_ext != NULL)
                cl_ext->words = words;

        return cl_ext;
#else
        return NULL;
#endif
}

Most arches are 64bit so once one label is active we already allocate 16 bytes
due to the padding hole in nf_conn_labels struct.

OVS always asks for 128bit so in that case we'd only allocate 16 instead of the
current 24 byte.

Re: [PATCH nf] netfilter: nft_ct: fix unpaired nf_connlabels_get/put call

[Pablo Neira Ayuso]

On Sat, Jul 16, 2016 at 11:18:01AM +0200, Florian Westphal wrote:
> Liping Zhang <zlpnobody@163.com> wrote:
> > From: Liping Zhang <liping.zhang@spreadtrum.com>
> > 
> > We only get nf_connlabels if the user add ct label set expr successfully,
> > but we will also put nf_connlabels if the user delete ct lable get expr.
> > This is mismathced, and will cause ct label expr cannot work properly.
> > 
> > Also, if we init something fail, we should put nf_connlabels back.
> > Otherwise, we may waste to alloc the memory that will never be used.
> 
> Acked-by: Florian Westphal <fw@strlen.de>
> 
> Unrelated to your patch:
> 
> I think its time to change conntrack labels to a pure 128 bit field:

I think this is going to simplify this code a bit, so go ahead with this.