Re: [PATCH] can: fix warning in bcm_connect/proc_register
On 10/27/2016 06:28 PM, Cong Wang wrote: >>> Hmm, bo->bound should guarantee it, so never mind, your patch >>> looks fine. >> >> Can I add your Acked-by? > > Of course. > > Acked-by: Cong WangThanks, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH] can: fix warning in bcm_connect/proc_register
On Thu, Oct 27, 2016 at 1:45 AM, Marc Kleine-Buddewrote: > On 10/24/2016 10:17 PM, Cong Wang wrote: >> On Mon, Oct 24, 2016 at 1:10 PM, Cong Wang wrote: >>> On Mon, Oct 24, 2016 at 12:11 PM, Oliver Hartkopp >>> wrote: 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, _proc_fops, sk); + 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. >> >> 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 Thanks.
Re: [PATCH] can: fix warning in bcm_connect/proc_register
On 10/24/2016 09:11 PM, Oliver Hartkopp wrote: > 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=147732648731237 > > Reported-by: Andrey Konovalov> Suggested-by: Cong Wang > Signed-off-by: Oliver Hartkopp Added with Andrey Konovalov Tested-by to can/master with stable on Cc. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH] can: fix warning in bcm_connect/proc_register
On 10/24/2016 10:17 PM, Cong Wang wrote: > On Mon, Oct 24, 2016 at 1:10 PM, Cong Wangwrote: >> On Mon, Oct 24, 2016 at 12:11 PM, Oliver Hartkopp >> wrote: >>> 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, >>> _proc_fops, sk); >>> + 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. > > Hmm, bo->bound should guarantee it, so never mind, your patch > looks fine. Can I add your Acked-by? Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH] can: fix warning in bcm_connect/proc_register
Hi Oliver, I can confirm that your patch fixes the warnings for me. Tested-by: Andrey KonovalovOn Mon, Oct 24, 2016 at 10:17 PM, Cong Wang wrote: > On Mon, Oct 24, 2016 at 1:10 PM, Cong Wang wrote: >> On Mon, Oct 24, 2016 at 12:11 PM, Oliver Hartkopp >> wrote: >>> 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, >>> _proc_fops, sk); >>> + 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. > > Hmm, bo->bound should guarantee it, so never mind, your patch > looks fine.
Re: [PATCH] can: fix warning in bcm_connect/proc_register
On Mon, Oct 24, 2016 at 1:10 PM, Cong Wangwrote: > On Mon, Oct 24, 2016 at 12:11 PM, Oliver Hartkopp > wrote: >> 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, >> _proc_fops, sk); >> + 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. Hmm, bo->bound should guarantee it, so never mind, your patch looks fine.
Re: [PATCH] can: fix warning in bcm_connect/proc_register
On Mon, Oct 24, 2016 at 12:11 PM, Oliver Hartkoppwrote: > 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, > _proc_fops, sk); > + 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.
[PATCH] 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=147732648731237 Reported-by: Andrey KonovalovSuggested-by: Cong Wang Signed-off-by: Oliver Hartkopp --- net/can/bcm.c | 32 +++- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/net/can/bcm.c b/net/can/bcm.c index 8e999ff..8af9d25 100644 --- a/net/can/bcm.c +++ b/net/can/bcm.c @@ -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(_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, _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, -- 2.9.3