can: fix warning in bcm_connect/proc_register

PATCH In review Oliver Hartkopp 4 participants 8 messages Oct 24, 2016 – Oct 28, 2016

Subsystems (4)

Networking Network drivers CAN network drivers CAN network layer

lore

Share & tools

lore

Oliver Hartkopp

author maintainer

2016-10-24 19:11 UTC #0 lore raw

PATCH 1/1 can: fix warning in bcm_connect/proc_register

Andrey Konovalov reported an issue with proc_register in bcm.c. As suggested by Cong Wang this patch adds a lock_sock() protection and a check for unsuccessful proc_create_data() in bcm_connect().

Reference: http://marc.info/?l=linux-netdev&m=147732648731237

Reported-by Andrey Konovalov <[email protected]>

Suggested-by Cong Wang <[email protected]>

Signed-off-by Oliver Hartkopp <[email protected]>

---
 net/can/bcm.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

net/can/bcm.c +23 -9

index 8e999ff..8af9d25 100644
@@ -1549,24 +1549,31 @@ static int bcm_connect(struct socket *sock, struct sockaddr *uaddr, int len,
 	struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
 	struct sock *sk = sock->sk;
 	struct bcm_sock *bo = bcm_sk(sk);
+	int ret = 0;
 
 	if (len < sizeof(*addr))
 		return -EINVAL;
 
-	if (bo->bound)
-		return -EISCONN;
+	lock_sock(sk);
+
+	if (bo->bound) {
+		ret = -EISCONN;
+		goto fail;
+	}
 
 	/* bind a device to this socket */
 	if (addr->can_ifindex) {
 		struct net_device *dev;
 
 		dev = dev_get_by_index(&init_net, addr->can_ifindex);
-		if (!dev)
-			return -ENODEV;
-
+		if (!dev) {
+			ret = -ENODEV;
+			goto fail;
+		}
 		if (dev->type != ARPHRD_CAN) {
 			dev_put(dev);
-			return -ENODEV;
+			ret = -ENODEV;
+			goto fail;
 		}
 
 		bo->ifindex = dev->ifindex;

@@ -1577,17 +1584,24 @@ static int bcm_connect(struct socket *sock, struct sockaddr *uaddr, int len,
 		bo->ifindex = 0;
 	}
 
-	bo->bound = 1;
-
 	if (proc_dir) {
 		/* unique socket address as filename */
 		sprintf(bo->procname, "%lu", sock_i_ino(sk));
 		bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
 						     proc_dir,
 						     &bcm_proc_fops, sk);
+		if (!bo->bcm_proc_read) {
+			ret = -ENOMEM;
+			goto fail;
+		}
 	}
 
-	return 0;
+	bo->bound = 1;
+
+fail:
+	release_sock(sk);
+
+	return ret;
 }
 
 static int bcm_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,

Signature · 1 line

2.9.3

Cong Wang

2016-10-24 20:10 UTC #1 lore raw

net/can/bcm.c static int bcm_connect(struct socket *sock, struct sockaddr *uaddr, int len, · line 1596

Quoted 10 lines · Oliver Hartkopp

        if (proc_dir) {
                /* unique socket address as filename */
                sprintf(bo->procname, "%lu", sock_i_ino(sk));
                bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
                                                     proc_dir,
                                                     &bcm_proc_fops, sk);

net/can/bcm.c +4 -0

+               if (!bo->bcm_proc_read) {
+                       ret = -ENOMEM;
+                       goto fail;
+               }

Well, I meant we need to call proc_create_data() once per socket, so we need a check before proc_create_data() too.

Thanks.

Cong Wang

2016-10-24 20:17 UTC #2 lore raw

Quoted 13 lines · Cong Wang

net/can/bcm.c static int bcm_connect(struct socket *sock, struct sockaddr *uaddr, int len, · line 1596

Quoted 10 lines · Oliver Hartkopp

        if (proc_dir) {
                /* unique socket address as filename */
                sprintf(bo->procname, "%lu", sock_i_ino(sk));
                bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
                                                     proc_dir,
                                                     &bcm_proc_fops, sk);

net/can/bcm.c +4 -0

+               if (!bo->bcm_proc_read) {
+                       ret = -ENOMEM;
+                       goto fail;
+               }

Quoted 2 lines

Well, I meant we need to call proc_create_data() once per socket, so we need a check before proc_create_data() too.

Hmm, bo->bound should guarantee it, so never mind, your patch looks fine.

Andrey Konovalov

Tested-by

2016-10-25 12:12 UTC #3 lore raw

Hi Oliver,

I can confirm that your patch fixes the warnings for me.

Tested-by Andrey Konovalov <[email protected]>

Quoted 19 lines · Cong Wang

Quoted 13 lines · Cong Wang
net/can/bcm.c static int bcm_connect(struct socket *sock, struct sockaddr *uaddr, int len, · line 1596
Quoted 10 lines · Oliver Hartkopp
        if (proc_dir) {
                /* unique socket address as filename */
                sprintf(bo->procname, "%lu", sock_i_ino(sk));
                bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
                                                     proc_dir,
                                                     &bcm_proc_fops, sk);
net/can/bcm.c +4 -0
+               if (!bo->bcm_proc_read) {
+                       ret = -ENOMEM;
+                       goto fail;
+               }
Quoted 2 lines
Well, I meant we need to call proc_create_data() once per socket, so we need a check before proc_create_data() too.
Hmm, bo->bound should guarantee it, so never mind, your patch looks fine.

Marc Kleine-Budde

maintainer

2016-10-27 08:45 UTC #4 lore raw

In reply to Cong Wang

Quoted 14 lines · Cong Wang

Quoted 13 lines · Cong Wang

Quoted 10 lines · Oliver Hartkopp

        if (proc_dir) {
                /* unique socket address as filename */
                sprintf(bo->procname, "%lu", sock_i_ino(sk));
                bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
                                                     proc_dir,
                                                     &bcm_proc_fops, sk);

net/can/bcm.c +4 -0

+               if (!bo->bcm_proc_read) {
+                       ret = -ENOMEM;
+                       goto fail;
+               }

Quoted 5 lines

Quoted 2 lines
Well, I meant we need to call proc_create_data() once per socket, so we need a check before proc_create_data() too.
Hmm, bo->bound should guarantee it, so never mind, your patch looks fine.

Can I add your Acked-by?

Marc

Signature · 4 lines

Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Cong Wang

Acked-by

2016-10-27 16:28 UTC #5 lore raw

Quoted 15 lines · Marc Kleine-Budde

Quoted 14 lines · Cong Wang

Quoted 13 lines · Cong Wang

Quoted 10 lines · Oliver Hartkopp

        if (proc_dir) {
                /* unique socket address as filename */
                sprintf(bo->procname, "%lu", sock_i_ino(sk));
                bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
                                                     proc_dir,
                                                     &bcm_proc_fops, sk);

net/can/bcm.c +4 -0

+               if (!bo->bcm_proc_read) {
+                       ret = -ENOMEM;
+                       goto fail;
+               }

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Well, I meant we need to call proc_create_data() once per socket, so we need a check before proc_create_data() too.
Hmm, bo->bound should guarantee it, so never mind, your patch looks fine.
Can I add your Acked-by?

Of course.

Acked-by Cong Wang <[email protected]>

Thanks.

Marc Kleine-Budde

maintainer

2016-10-28 08:58 UTC #6 lore raw

Quoted 8 lines · Cong Wang

Quoted 4 lines
Quoted 2 lines
Hmm, bo->bound should guarantee it, so never mind, your patch looks fine.
Can I add your Acked-by?
Of course.
Acked-by Cong Wang <[email protected]>

Thanks,
Marc

Signature · 4 lines

Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Marc Kleine-Budde

maintainer

2016-10-27 08:47 UTC #7 lore raw

In reply to Oliver Hartkopp

Quoted 9 lines · Oliver Hartkopp

Andrey Konovalov reported an issue with proc_register in bcm.c. As suggested by Cong Wang this patch adds a lock_sock() protection and a check for unsuccessful proc_create_data() in bcm_connect().
Reference: http://marc.info/?l=linux-netdev&m=147732648731237
Reported-by Andrey Konovalov <[email protected]>
Suggested-by Cong Wang <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>

Added with Andrey Konovalov Tested-by to can/master with stable on Cc.

Marc

Signature · 4 lines

Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |