[PATCH 0/5] GTP: Fine-tuning for some function implementations

[PATCH 0/5] GTP: Fine-tuning for some function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 26 Jan 2017 11:10:01 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  Use kmalloc_array() in gtp_hashtable_new()
  Improve another size determination in ipv4_pdp_add()
  Adjust 12 checks for null pointers
  Rename jump labels in gtp_encap_enable()
  Rename jump labels in gtp_hashtable_new()

 drivers/net/gtp.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

-- 
2.11.0

[PATCH 1/5] gtp: Use kmalloc_array() in gtp_hashtable_new()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 25 Jan 2017 22:01:00 +0100

* Multiplications for the size determination of memory allocations
  indicated that array data structures should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of data structures by pointer dereferences
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/gtp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 8b6810bad54b..5d0d520ae40f 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -783,11 +783,13 @@ static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize)
 {
 	int i;
 
-	gtp->addr_hash = kmalloc(sizeof(struct hlist_head) * hsize, GFP_KERNEL);
+	gtp->addr_hash = kmalloc_array(hsize, sizeof(*gtp->addr_hash),
+				       GFP_KERNEL);
 	if (gtp->addr_hash == NULL)
 		return -ENOMEM;
 
-	gtp->tid_hash = kmalloc(sizeof(struct hlist_head) * hsize, GFP_KERNEL);
+	gtp->tid_hash = kmalloc_array(hsize, sizeof(*gtp->tid_hash),
+				      GFP_KERNEL);
 	if (gtp->tid_hash == NULL)
 		goto err1;
 
-- 
2.11.0

[PATCH 2/5] gtp: Improve another size determination in ipv4_pdp_add()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 25 Jan 2017 22:24:08 +0100

Replace the specification of a data type by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/gtp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 5d0d520ae40f..f0c123101aa9 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -956,7 +956,7 @@ static int ipv4_pdp_add(struct net_device *dev, struct genl_info *info)
 
 	}
 
-	pctx = kmalloc(sizeof(struct pdp_ctx), GFP_KERNEL);
+	pctx = kmalloc(sizeof(*pctx), GFP_KERNEL);
 	if (pctx == NULL)
 		return -ENOMEM;
 
-- 
2.11.0

[PATCH 3/5] gtp: Adjust 12 checks for null pointers

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 25 Jan 2017 22:50:23 +0100
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script "checkpatch.pl" pointed information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/gtp.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index f0c123101aa9..7e38786361d8 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -785,12 +785,12 @@ static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize)
 
 	gtp->addr_hash = kmalloc_array(hsize, sizeof(*gtp->addr_hash),
 				       GFP_KERNEL);
-	if (gtp->addr_hash == NULL)
+	if (!gtp->addr_hash)
 		return -ENOMEM;
 
 	gtp->tid_hash = kmalloc_array(hsize, sizeof(*gtp->tid_hash),
 				      GFP_KERNEL);
-	if (gtp->tid_hash == NULL)
+	if (!gtp->tid_hash)
 		goto err1;
 
 	gtp->hash_size = hsize;
@@ -832,7 +832,7 @@ static int gtp_encap_enable(struct net_device *dev, struct gtp_dev *gtp,
 	netdev_dbg(dev, "enable gtp on %d, %d\n", fd_gtp0, fd_gtp1);
 
 	sock0 = sockfd_lookup(fd_gtp0, &err);
-	if (sock0 == NULL) {
+	if (!sock0) {
 		netdev_dbg(dev, "socket fd=%d not found (gtp0)\n", fd_gtp0);
 		return -ENOENT;
 	}
@@ -844,7 +844,7 @@ static int gtp_encap_enable(struct net_device *dev, struct gtp_dev *gtp,
 	}
 
 	sock1u = sockfd_lookup(fd_gtp1, &err);
-	if (sock1u == NULL) {
+	if (!sock1u) {
 		netdev_dbg(dev, "socket fd=%d not found (gtp1u)\n", fd_gtp1);
 		err = -ENOENT;
 		goto err1;
@@ -957,7 +957,7 @@ static int ipv4_pdp_add(struct net_device *dev, struct genl_info *info)
 	}
 
 	pctx = kmalloc(sizeof(*pctx), GFP_KERNEL);
-	if (pctx == NULL)
+	if (!pctx)
 		return -ENOMEM;
 
 	ipv4_pdp_fill(pctx, info);
@@ -1029,7 +1029,7 @@ static int gtp_genl_new_pdp(struct sk_buff *skb, struct genl_info *info)
 
 	/* Check if there's an existing gtpX device to configure */
 	dev = gtp_find_dev(net, nla_get_u32(info->attrs[GTPA_LINK]));
-	if (dev == NULL) {
+	if (!dev) {
 		put_net(net);
 		return -ENODEV;
 	}
@@ -1055,7 +1055,7 @@ static int gtp_genl_del_pdp(struct sk_buff *skb, struct genl_info *info)
 
 	/* Check if there's an existing gtpX device to configure */
 	dev = gtp_find_dev(net, nla_get_u32(info->attrs[GTPA_LINK]));
-	if (dev == NULL) {
+	if (!dev) {
 		put_net(net);
 		return -ENODEV;
 	}
@@ -1079,7 +1079,7 @@ static int gtp_genl_del_pdp(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 	}
 
-	if (pctx == NULL)
+	if (!pctx)
 		return -ENOENT;
 
 	if (pctx->gtp_version == GTP_V0)
@@ -1105,7 +1105,7 @@ static int gtp_genl_fill_info(struct sk_buff *skb, u32 snd_portid, u32 snd_seq,
 
 	genlh = genlmsg_put(skb, snd_portid, snd_seq, &gtp_genl_family, 0,
 			    type);
-	if (genlh == NULL)
+	if (!genlh)
 		goto nlmsg_failure;
 
 	if (nla_put_u32(skb, GTPA_VERSION, pctx->gtp_version) ||
@@ -1163,7 +1163,7 @@ static int gtp_genl_get_pdp(struct sk_buff *skb, struct genl_info *info)
 
 	/* Check if there's an existing gtpX device to configure */
 	dev = gtp_find_dev(net, nla_get_u32(info->attrs[GTPA_LINK]));
-	if (dev == NULL) {
+	if (!dev) {
 		put_net(net);
 		return -ENODEV;
 	}
@@ -1188,13 +1188,13 @@ static int gtp_genl_get_pdp(struct sk_buff *skb, struct genl_info *info)
 		pctx = ipv4_pdp_find(gtp, ip);
 	}
 
-	if (pctx == NULL) {
+	if (!pctx) {
 		err = -ENOENT;
 		goto err_unlock;
 	}
 
 	skb2 = genlmsg_new(NLMSG_GOODSIZE, GFP_ATOMIC);
-	if (skb2 == NULL) {
+	if (!skb2) {
 		err = -ENOMEM;
 		goto err_unlock;
 	}
-- 
2.11.0

[PATCH 4/5] gtp: Rename jump labels in gtp_encap_enable()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 26 Jan 2017 10:48:40 +0100

Adjust jump labels according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/gtp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 7e38786361d8..2d4ddc000919 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -840,20 +840,20 @@ static int gtp_encap_enable(struct net_device *dev, struct gtp_dev *gtp,
 	if (sock0->sk->sk_protocol != IPPROTO_UDP) {
 		netdev_dbg(dev, "socket fd=%d not UDP\n", fd_gtp0);
 		err = -EINVAL;
-		goto err1;
+		goto put_socket;
 	}
 
 	sock1u = sockfd_lookup(fd_gtp1, &err);
 	if (!sock1u) {
 		netdev_dbg(dev, "socket fd=%d not found (gtp1u)\n", fd_gtp1);
 		err = -ENOENT;
-		goto err1;
+		goto put_socket;
 	}
 
 	if (sock1u->sk->sk_protocol != IPPROTO_UDP) {
 		netdev_dbg(dev, "socket fd=%d not UDP\n", fd_gtp1);
 		err = -EINVAL;
-		goto err2;
+		goto put_socket_u;
 	}
 
 	netdev_dbg(dev, "enable gtp on %p, %p\n", sock0, sock1u);
@@ -873,9 +873,9 @@ static int gtp_encap_enable(struct net_device *dev, struct gtp_dev *gtp,
 	setup_udp_tunnel_sock(sock_net(gtp->sock1u->sk), gtp->sock1u, &tuncfg);
 
 	err = 0;
-err2:
+put_socket_u:
 	sockfd_put(sock1u);
-err1:
+put_socket:
 	sockfd_put(sock0);
 	return err;
 }
-- 
2.11.0

[PATCH 5/5] gtp: Rename jump labels in gtp_hashtable_new()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 26 Jan 2017 10:54:20 +0100

Adjust a jump label according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/gtp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 2d4ddc000919..1571d6c7fe81 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -791,7 +791,7 @@ static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize)
 	gtp->tid_hash = kmalloc_array(hsize, sizeof(*gtp->tid_hash),
 				      GFP_KERNEL);
 	if (!gtp->tid_hash)
-		goto err1;
+		goto free_hash;
 
 	gtp->hash_size = hsize;
 
@@ -800,7 +800,7 @@ static int gtp_hashtable_new(struct gtp_dev *gtp, int hsize)
 		INIT_HLIST_HEAD(&gtp->tid_hash[i]);
 	}
 	return 0;
-err1:
+free_hash:
 	kfree(gtp->addr_hash);
 	return -ENOMEM;
 }
-- 
2.11.0

Re: [PATCH 0/5] GTP: Fine-tuning for some function implementations

[Andreas Schultz]

Hi Markus,

----- On Jan 26, 2017, at 11:15 AM, SF Markus Elfring elfring@users.sourceforge.net wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 26 Jan 2017 11:10:01 +0100
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (5):
>  Use kmalloc_array() in gtp_hashtable_new()
>  Improve another size determination in ipv4_pdp_add()
>  Adjust 12 checks for null pointers
>  Rename jump labels in gtp_encap_enable()
>  Rename jump labels in gtp_hashtable_new()

Looks good to me, for all the above:

Reviewed-by: Andreas Schultz <aschultz@tpip.net>

Andreas

> 
> drivers/net/gtp.c | 46 ++++++++++++++++++++++++----------------------
> 1 file changed, 24 insertions(+), 22 deletions(-)
> 
> --
> 2.11.0

Re: [PATCH 2/5] gtp: Improve another size determination in ipv4_pdp_add()

[Alexey Dobriyan]

On Thu, Jan 26, 2017 at 1:18 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:

> -       pctx = kmalloc(sizeof(struct pdp_ctx), GFP_KERNEL);
> +       pctx = kmalloc(sizeof(*pctx), GFP_KERNEL);

The rule about "sizeof(*p)" style of allocation is bogus and should be
abolished.

Rationale says that

    > The alternative form where struct name is spelled out hurts readability

In terms of line length, yes, "sizeof(*p)" wins most of the time.
However, the former variant clearly show the type of data, so you don't need
to look it up and makes it readily available to follow with tags or equivalent.

    > and introduces an opportunity for a bug when the pointer variable type
    > is changed but the corresponding sizeof that is passed to a memory
    > allocator is not.

The correct way to prevent this kind of mistake is to not return "void
*" pointer
(at least most of the uses use typed allocation)

    #define lmalloc(T, gfp) (T*)_kmalloc(sizeof(T), (gfp))

Bacause of a cast changing type of pointer will be noticed,