[PATCH 1/4] netfilter: xt_connlimit: fix daddr connlimit in SNAT scenario

[PATCH 1/4] netfilter: xt_connlimit: fix daddr connlimit in SNAT scenario

[Changli Gao]

We use the reply tuples when limiting the connections by the destination
addresses, however, in SNAT scenario, the final reply tuples won't be
ready until SNAT is done in POSTROUING or INPUT chain, and the following
nf_conntrack_find_get() in count_tem() will get nothing, so connlimit
can't work as expected.

In this patch, the original tuples are always used, and an additional
member addr is appended to save the address in either end.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 net/netfilter/xt_connlimit.c |   20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index e029c48..1f4b9f9 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -33,8 +33,9 @@
 
 /* we will save the tuples of all connections we care about */
 struct xt_connlimit_conn {
-	struct list_head list;
-	struct nf_conntrack_tuple tuple;
+	struct list_head		list;
+	struct nf_conntrack_tuple	tuple;
+	union nf_inet_addr		addr;
 };
 
 struct xt_connlimit_data {
@@ -151,7 +152,7 @@ static int count_them(struct net *net,
 			continue;
 		}
 
-		if (same_source_net(addr, mask, &conn->tuple.src.u3, family))
+		if (same_source_net(addr, mask, &conn->addr, family))
 			/* same source network -> be counted! */
 			++matches;
 		nf_ct_put(found_ct);
@@ -165,6 +166,7 @@ static int count_them(struct net *net,
 		if (conn == NULL)
 			return -ENOMEM;
 		conn->tuple = *tuple;
+		conn->addr = *addr;
 		list_add(&conn->list, hash);
 		++matches;
 	}
@@ -185,15 +187,11 @@ connlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	int connections;
 
 	ct = nf_ct_get(skb, &ctinfo);
-	if (ct != NULL) {
-		if (info->flags & XT_CONNLIMIT_DADDR)
-			tuple_ptr = &ct->tuplehash[IP_CT_DIR_REPLY].tuple;
-		else
-			tuple_ptr = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
-	} else if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
-				    par->family, &tuple)) {
+	if (ct != NULL)
+		tuple_ptr = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
+	else if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
+				    par->family, &tuple))
 		goto hotdrop;
-	}
 
 	if (par->family == NFPROTO_IPV6) {
 		const struct ipv6hdr *iph = ipv6_hdr(skb);

[PATCH 2/4] netfilter: xt_connlimit: use kmalloc() instead of kzalloc()

[Changli Gao]

All the members are initialized after kzalloc().

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 net/netfilter/xt_connlimit.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index 1f4b9f9..ade2a80 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -162,7 +162,7 @@ static int count_them(struct net *net,
 
 	if (addit) {
 		/* save the new connection in our list */
-		conn = kzalloc(sizeof(*conn), GFP_ATOMIC);
+		conn = kmalloc(sizeof(*conn), GFP_ATOMIC);
 		if (conn == NULL)
 			return -ENOMEM;
 		conn->tuple = *tuple;

[PATCH 3/4] netfilter: xt_connlimit: use hlist instead

[Changli Gao]

The header of hlist is smaller than list.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 net/netfilter/xt_connlimit.c |   28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index ade2a80..da56d6e 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -33,14 +33,14 @@
 
 /* we will save the tuples of all connections we care about */
 struct xt_connlimit_conn {
-	struct list_head		list;
+	struct hlist_node		node;
 	struct nf_conntrack_tuple	tuple;
 	union nf_inet_addr		addr;
 };
 
 struct xt_connlimit_data {
-	struct list_head iphash[256];
-	spinlock_t lock;
+	struct hlist_head	iphash[256];
+	spinlock_t		lock;
 };
 
 static u_int32_t connlimit_rnd __read_mostly;
@@ -102,9 +102,9 @@ static int count_them(struct net *net,
 {
 	const struct nf_conntrack_tuple_hash *found;
 	struct xt_connlimit_conn *conn;
-	struct xt_connlimit_conn *tmp;
+	struct hlist_node *pos, *n;
 	struct nf_conn *found_ct;
-	struct list_head *hash;
+	struct hlist_head *hash;
 	bool addit = true;
 	int matches = 0;
 
@@ -116,7 +116,7 @@ static int count_them(struct net *net,
 	rcu_read_lock();
 
 	/* check the saved connections */
-	list_for_each_entry_safe(conn, tmp, hash, list) {
+	hlist_for_each_entry_safe(conn, pos, n, hash, node) {
 		found    = nf_conntrack_find_get(net, NF_CT_DEFAULT_ZONE,
 						 &conn->tuple);
 		found_ct = NULL;
@@ -136,7 +136,7 @@ static int count_them(struct net *net,
 
 		if (found == NULL) {
 			/* this one is gone */
-			list_del(&conn->list);
+			hlist_del(&conn->node);
 			kfree(conn);
 			continue;
 		}
@@ -147,7 +147,7 @@ static int count_them(struct net *net,
 			 * closed already -> ditch it
 			 */
 			nf_ct_put(found_ct);
-			list_del(&conn->list);
+			hlist_del(&conn->node);
 			kfree(conn);
 			continue;
 		}
@@ -167,7 +167,7 @@ static int count_them(struct net *net,
 			return -ENOMEM;
 		conn->tuple = *tuple;
 		conn->addr = *addr;
-		list_add(&conn->list, hash);
+		hlist_add_head(&conn->node, hash);
 		++matches;
 	}
 
@@ -246,7 +246,7 @@ static int connlimit_mt_check(const struct xt_mtchk_param *par)
 
 	spin_lock_init(&info->data->lock);
 	for (i = 0; i < ARRAY_SIZE(info->data->iphash); ++i)
-		INIT_LIST_HEAD(&info->data->iphash[i]);
+		INIT_HLIST_HEAD(&info->data->iphash[i]);
 
 	return 0;
 }
@@ -255,15 +255,15 @@ static void connlimit_mt_destroy(const struct xt_mtdtor_param *par)
 {
 	const struct xt_connlimit_info *info = par->matchinfo;
 	struct xt_connlimit_conn *conn;
-	struct xt_connlimit_conn *tmp;
-	struct list_head *hash = info->data->iphash;
+	struct hlist_node *pos, *n;
+	struct hlist_head *hash = info->data->iphash;
 	unsigned int i;
 
 	nf_ct_l3proto_module_put(par->family);
 
 	for (i = 0; i < ARRAY_SIZE(info->data->iphash); ++i) {
-		list_for_each_entry_safe(conn, tmp, &hash[i], list) {
-			list_del(&conn->list);
+		hlist_for_each_entry_safe(conn, pos, n, &hash[i], node) {
+			hlist_del(&conn->node);
 			kfree(conn);
 		}
 	}

[PATCH 4/4] netfilter: xt_connlimit: remove connlimit_rnd_inited

[Changli Gao]

A potential race condition when generating connlimit_rnd is also fixed.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 net/netfilter/xt_connlimit.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index da56d6e..c6d5a83 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -44,7 +44,6 @@ struct xt_connlimit_data {
 };
 
 static u_int32_t connlimit_rnd __read_mostly;
-static bool connlimit_rnd_inited __read_mostly;
 
 static inline unsigned int connlimit_iphash(__be32 addr)
 {
@@ -226,9 +225,13 @@ static int connlimit_mt_check(const struct xt_mtchk_param *par)
 	unsigned int i;
 	int ret;
 
-	if (unlikely(!connlimit_rnd_inited)) {
-		get_random_bytes(&connlimit_rnd, sizeof(connlimit_rnd));
-		connlimit_rnd_inited = true;
+	if (unlikely(!connlimit_rnd)) {
+		u_int32_t rand;
+
+		do {
+			get_random_bytes(&rand, sizeof(rand));
+		} while (!rand);
+		cmpxchg(&connlimit_rnd, 0, rand);
 	}
 	ret = nf_ct_l3proto_try_module_get(par->family);
 	if (ret < 0) {

Re: [PATCH 1/4] netfilter: xt_connlimit: fix daddr connlimit in SNAT scenario

[Jan Engelhardt]

On Monday 2011-03-14 07:50, Changli Gao wrote:

>We use the reply tuples when limiting the connections by the destination
>addresses, however, in SNAT scenario, the final reply tuples won't be
>ready until SNAT is done in POSTROUING or INPUT chain

If I am not mistaken: if you do daddr counting, SNAT is irrelevant.
Consider ruleset
 -t nat -A PREROUTING -p tcp --dport 80 -j DNAT --to 1.2.3.4:80
 -t nat -A PREROUTING -p tcp --dport 443 -j DNAT --to 1.2.3.5:443

The tuple will first be (as per conntrack -L):
 src=home dst=router src=router dst=home
After DNAT:
 src=home dst=router src=1.2.3.4 dst=home

Thus looking at the src of the reply tuple seems correct — at least this 
is what was wanted, counting per stashed servers (=1 customer), not per 
globally visible address.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] netfilter: xt_connlimit: fix daddr connlimit in SNAT scenario

[Changli Gao]

On Mon, Mar 14, 2011 at 8:26 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
> On Monday 2011-03-14 07:50, Changli Gao wrote:
>
>>We use the reply tuples when limiting the connections by the destination
>>addresses, however, in SNAT scenario, the final reply tuples won't be
>>ready until SNAT is done in POSTROUING or INPUT chain
>
> If I am not mistaken: if you do daddr counting, SNAT is irrelevant.
> Consider ruleset
>  -t nat -A PREROUTING -p tcp --dport 80 -j DNAT --to 1.2.3.4:80
>  -t nat -A PREROUTING -p tcp --dport 443 -j DNAT --to 1.2.3.5:443
>
> The tuple will first be (as per conntrack -L):
>  src=home dst=router src=router dst=home
> After DNAT:
>  src=home dst=router src=1.2.3.4 dst=home
>
> Thus looking at the src of the reply tuple seems correct — at least this
> is what was wanted, counting per stashed servers (=1 customer), not per
> globally visible address.
>

Yes, you are correct only when there is no SNAT rule. If there is an SNAT rule:

-t nat -A POSTROUTING -p tcp --dport 80 -j SNAT --to-source 192.168.0.1

the final tuples will be:
src = home dst = router src=1.2.3.4 dst=192.168.0.1

However, the tuple saved by connlimit is src=1.2.3.4 dst=home, so this
conn will be removed later as there isn't any conntrack, which has
this tuple in any direction.

You can't prevent a user from doing such a configuration, although you
might think it is stupid to do that.

Thanks for your review.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 1/4] netfilter: xt_connlimit: fix daddr connlimit in SNAT scenario

[Patrick McHardy]

On 14.03.2011 13:42, Changli Gao wrote:
> On Mon, Mar 14, 2011 at 8:26 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>> On Monday 2011-03-14 07:50, Changli Gao wrote:
>>
>>> We use the reply tuples when limiting the connections by the destination
>>> addresses, however, in SNAT scenario, the final reply tuples won't be
>>> ready until SNAT is done in POSTROUING or INPUT chain
>>
>> If I am not mistaken: if you do daddr counting, SNAT is irrelevant.
>> Consider ruleset
>>  -t nat -A PREROUTING -p tcp --dport 80 -j DNAT --to 1.2.3.4:80
>>  -t nat -A PREROUTING -p tcp --dport 443 -j DNAT --to 1.2.3.5:443
>>
>> The tuple will first be (as per conntrack -L):
>>  src=home dst=router src=router dst=home
>> After DNAT:
>>  src=home dst=router src=1.2.3.4 dst=home
>>
>> Thus looking at the src of the reply tuple seems correct — at least this
>> is what was wanted, counting per stashed servers (=1 customer), not per
>> globally visible address.
>>
> 
> Yes, you are correct only when there is no SNAT rule. If there is an SNAT rule:
> 
> -t nat -A POSTROUTING -p tcp --dport 80 -j SNAT --to-source 192.168.0.1
> 
> the final tuples will be:
> src = home dst = router src=1.2.3.4 dst=192.168.0.1
> 
> However, the tuple saved by connlimit is src=1.2.3.4 dst=home, so this
> conn will be removed later as there isn't any conntrack, which has
> this tuple in any direction.
> 
> You can't prevent a user from doing such a configuration, although you
> might think it is stupid to do that.
> 
> Thanks for your review.

Jan, please let me know whether you want me to apply these patches.
Thanks.

Re: [PATCH 1/4] netfilter: xt_connlimit: fix daddr connlimit in SNAT scenario

[Jan Engelhardt]

On Monday 2011-03-14 13:42, Changli Gao wrote:

>On Mon, Mar 14, 2011 at 8:26 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>> On Monday 2011-03-14 07:50, Changli Gao wrote:
>>
>>>We use the reply tuples when limiting the connections by the destination
>>>addresses, however, in SNAT scenario, the final reply tuples won't be
>>>ready until SNAT is done in POSTROUING or INPUT chain
>>
>> If I am not mistaken: if you do daddr counting, SNAT is irrelevant.
>> Consider ruleset
>>  -t nat -A PREROUTING -p tcp --dport 80 -j DNAT --to 1.2.3.4:80
>>  -t nat -A PREROUTING -p tcp --dport 443 -j DNAT --to 1.2.3.5:443
>>
>> The tuple will first be (as per conntrack -L):
>>  src=home dst=router src=router dst=home
>> After DNAT:
>>  src=home dst=router src=1.2.3.4 dst=home
>>
>> Thus looking at the src of the reply tuple seems correct — at least this
>> is what was wanted, counting per stashed servers (=1 customer), not per
>> globally visible address.
>>
>
>Yes, you are correct only when there is no SNAT rule. If there is an 
>SNAT rule:
>
>-t nat -A POSTROUTING -p tcp --dport 80 -j SNAT --to-source 192.168.0.1
>
>the final tuples will be:
>src = home dst = router src=1.2.3.4 dst=192.168.0.1
>
>However, the tuple saved by connlimit is src=1.2.3.4 dst=home, so this
>conn will be removed later as there isn't any conntrack, which has
>this tuple in any direction.

But I don't yet see how your patch #1 can help. At the time 
conn->tuple = *tuple is done, *tuple still contains the non-SNATed 
tuple, and it is never updated again.


(Patrick: patches 2-4 are ok)

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

Re: [PATCH 1/4] netfilter: xt_connlimit: fix daddr connlimit in SNAT scenario

[Changli Gao]

On Tue, Mar 15, 2011 at 3:00 AM, Jan Engelhardt <jengelh@medozas.de> wrote:
> On Monday 2011-03-14 13:42, Changli Gao wrote:
>
>>
>>Yes, you are correct only when there is no SNAT rule. If there is an
>>SNAT rule:
>>
>>-t nat -A POSTROUTING -p tcp --dport 80 -j SNAT --to-source 192.168.0.1
>>
>>the final tuples will be:
>>src = home dst = router src=1.2.3.4 dst=192.168.0.1
>>
>>However, the tuple saved by connlimit is src=1.2.3.4 dst=home, so this
>>conn will be removed later as there isn't any conntrack, which has
>>this tuple in any direction.
>
> But I don't yet see how your patch #1 can help. At the time
> conn->tuple = *tuple is done, *tuple still contains the non-SNATed
> tuple, and it is never updated again.
>

In this patch, conn->addr is used to save the destination/source
address instead of conn->tuple.src.u3, so the conn->tuple is used for
conntrack lookup only. Just as the original tuple isn't updated, we
can use it to looking up the associated conntrack all the time.

addr: 192.168.0.1
tuple: src = home, dst = router

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] netfilter: xt_connlimit: fix daddr connlimit in SNAT scenario

[Jan Engelhardt]

On Tuesday 2011-03-15 00:49, Changli Gao wrote:
>>>the final tuples will be:
>>>src = home dst = router src=1.2.3.4 dst=192.168.0.1
>>>
>>>However, the tuple saved by connlimit is src=1.2.3.4 dst=home, so this
>>>conn will be removed later as there isn't any conntrack, which has
>>>this tuple in any direction.
>>
>> But I don't yet see how your patch #1 can help. At the time
>> conn->tuple = *tuple is done, *tuple still contains the non-SNATed
>> tuple, and it is never updated again.
>
>In this patch, conn->addr is used to save the destination/source
>address instead of conn->tuple.src.u3, so the conn->tuple is used for
>conntrack lookup only. Just as the original tuple isn't updated, we
>can use it to looking up the associated conntrack all the time.

The original tuple may not be updated, but the reply tuple is.
And we are taking the reply tuple in

	tuple_ptr = &ct->tuplehash[IP_CT_DIR_REPLY].tuple;

which is subsequently copied to conn->tuple on the first invocation.

Afterwards, SNAT will update ct->tuplehash[reply].tuple, and so
conn->tuple is outdated. Calling nf_conntrack_find_get(conn->tuple)
in count_them would then fail, would it not?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] netfilter: xt_connlimit: fix daddr connlimit in SNAT scenario

[Changli Gao]

On Tue, Mar 15, 2011 at 9:16 AM, Jan Engelhardt <jengelh@medozas.de> wrote:
>
>
> The original tuple may not be updated, but the reply tuple is.
> And we are taking the reply tuple in
>
>        tuple_ptr = &ct->tuplehash[IP_CT_DIR_REPLY].tuple;
>
> which is subsequently copied to conn->tuple on the first invocation.
>
> Afterwards, SNAT will update ct->tuplehash[reply].tuple, and so
> conn->tuple is outdated. Calling nf_conntrack_find_get(conn->tuple)
> in count_them would then fail, would it not?
>

After my patch, tuple is only used to look up the corresponding
conntrack. And as you know, one conntrack has two tuples, you can look
up a conntrack by either one. The old implementation uses the
conn->tuple.src.u3 to save target address, so you need to use the
reply tuple to work around the DNAT issue. In my patch, the target
address isn't saved in conn->tuple.src.u3 but in conn->addr, so we can
use the constant tuple in the original direction to look up the
corresponding conntrack in count_them(). No reply tuple involved, no
failed nf_conntrack_find_get().

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] netfilter: xt_connlimit: fix daddr connlimit in SNAT scenario

[Jan Engelhardt]

On Tuesday 2011-03-15 02:30, Changli Gao wrote:

>On Tue, Mar 15, 2011 at 9:16 AM, Jan Engelhardt <jengelh@medozas.de> wrote:
>>
>>
>> The original tuple may not be updated, but the reply tuple is.
>> And we are taking the reply tuple in
>>
>>        tuple_ptr = &ct->tuplehash[IP_CT_DIR_REPLY].tuple;
>>
>> which is subsequently copied to conn->tuple on the first invocation.
>>
>> Afterwards, SNAT will update ct->tuplehash[reply].tuple, and so
>> conn->tuple is outdated. Calling nf_conntrack_find_get(conn->tuple)
>> in count_them would then fail, would it not?
>>
>
>After my patch, tuple is only used to look up the corresponding
>conntrack.

Ok, the patch may be applied. Somehow I was under the impression
addr was extracted from tuple_ptr, but it is, in fact, not.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] netfilter: xt_connlimit: fix daddr connlimit in SNAT scenario

[Patrick McHardy]

On 15.03.2011 03:05, Jan Engelhardt wrote:
> On Tuesday 2011-03-15 02:30, Changli Gao wrote:
> 
>> On Tue, Mar 15, 2011 at 9:16 AM, Jan Engelhardt <jengelh@medozas.de> wrote:
>>>
>>>
>>> The original tuple may not be updated, but the reply tuple is.
>>> And we are taking the reply tuple in
>>>
>>>        tuple_ptr = &ct->tuplehash[IP_CT_DIR_REPLY].tuple;
>>>
>>> which is subsequently copied to conn->tuple on the first invocation.
>>>
>>> Afterwards, SNAT will update ct->tuplehash[reply].tuple, and so
>>> conn->tuple is outdated. Calling nf_conntrack_find_get(conn->tuple)
>>> in count_them would then fail, would it not?
>>>
>>
>> After my patch, tuple is only used to look up the corresponding
>> conntrack.
> 
> Ok, the patch may be applied. Somehow I was under the impression
> addr was extracted from tuple_ptr, but it is, in fact, not.
> 

OK, thanks, I'll apply patches 1-4.

Re: [PATCH 1/4] netfilter: xt_connlimit: fix daddr connlimit in SNAT scenario

[Patrick McHardy]

On 14.03.2011 07:50, Changli Gao wrote:
> We use the reply tuples when limiting the connections by the destination
> addresses, however, in SNAT scenario, the final reply tuples won't be
> ready until SNAT is done in POSTROUING or INPUT chain, and the following
> nf_conntrack_find_get() in count_tem() will get nothing, so connlimit
> can't work as expected.
> 
> In this patch, the original tuples are always used, and an additional
> member addr is appended to save the address in either end.

Applied, thanks.

Re: [PATCH 2/4] netfilter: xt_connlimit: use kmalloc() instead of kzalloc()

[Patrick McHardy]

On 14.03.2011 07:50, Changli Gao wrote:
> All the members are initialized after kzalloc().

Applied.

Re: [PATCH 3/4] netfilter: xt_connlimit: use hlist instead

[Patrick McHardy]

On 14.03.2011 07:50, Changli Gao wrote:
> The header of hlist is smaller than list.

Applied.

Re: [PATCH 4/4] netfilter: xt_connlimit: remove connlimit_rnd_inited

[Patrick McHardy]

On 14.03.2011 07:50, Changli Gao wrote:
> A potential race condition when generating connlimit_rnd is also fixed.

Also applied, thanks Changli.