Hi Parav, Thanks for your feedback. As you mentioned below since addr4_resolve() or addr6_resolve() doesn't fail ,ndev is not NULL in mycase. And the below simpler patch which was provided by you holds good. I will update the patch and will resend the same. Will it be ok? Regards, Muneendra. -----Original Message----- From: Parav Pandit [mailto:parav@xxxxxxxxxxxx] Sent: Friday, February 23, 2018 1:06 AM To: Muneendra Kumar M <muneendra.kumar@xxxxxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx Subject: RE: [PATCH] Bug[RDMA/core]:Null pointer check is missing in addr_resolve Hi Muneendra, > -----Original Message----- > From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- > owner@xxxxxxxxxxxxxxx] On Behalf Of Muneendra Kumar M > Sent: Wednesday, February 21, 2018 4:21 AM > To: linux-rdma@xxxxxxxxxxxxxxx > Subject: [PATCH] Bug[RDMA/core]:Null pointer check is missing in > addr_resolve > > Null pointer check is missing in addr_resolve as dev_get_by_index may > return a NULL pointer. > And this patch will check whether ndev is NULL and further access the same > . > We observed the issue where the sytem crashed in the below code > > if (ndev->flags & IFF_LOOPBACK) > { > ret = rdma_translate_ip (dst_in, addr, NULL); > /* > * Put the loopback device and get the translated > * device instead. > */ > dev_put (ndev); > ndev = dev_get_by_index (addr->net, addr->bound_dev_if); Above ndev assignment is of no use. It just serves the purpose of keeping dev_put(). So a simpler patch would be possibly below. diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c index a039a49..4af043e 100644 --- a/drivers/infiniband/core/addr.c +++ b/drivers/infiniband/core/addr.c @@ -550,19 +550,13 @@ static int addr_resolve(struct sockaddr *src_in, dst_release(dst); } - if (ndev->flags & IFF_LOOPBACK) { - ret = rdma_translate_ip(dst_in, addr); - /* - * Put the loopback device and get the translated - * device instead. - */ + if (ndev) { + if (ndev->flags & IFF_LOOPBACK) + ret = rdma_translate_ip(dst_in, addr); + else + addr->bound_dev_if = ndev->ifindex; dev_put(ndev); - ndev = dev_get_by_index(addr->net, addr->bound_dev_if); - } else { - addr->bound_dev_if = ndev->ifindex; } - dev_put(ndev); - return ret; } > /*Bug: dev_get_by_index returns null*/ } else > { > addr->bound_dev_if = ndev->ifindex; > } > > dev_put (ndev) <== system crashed > And below is the crash > > [ 146.173149] BUG: unable to handle kernel NULL pointer dereference > at > 00000000000004a0 > [ 146.173198] IP: addr_resolve+0x9e/0x3e0 [ib_core] [ 146.173221] > PGD 0 P4D > 0 [ 146.173869] Oops: 0000 [#1] SMP PTI > [ 146.182859] CPU: 8 PID: 127 Comm: kworker/8:1 Tainted: G O > 4.15.0-rc6+ #18 > [ 146.183758] Hardware name: LENOVO System x3650 M5: -[8871AC1]- > /01KN179, > BIOS-[TCE132H-2.50]- 10/11/2017 > [ 146.184691] Workqueue: ib_cm cm_work_handler [ib_cm] [ 146.185632] > RIP: > 0010:addr_resolve+0x9e/0x3e0 [ib_core] [ 146.186584] RSP: > 0018:ffffc9000362faa0 EFLAGS: 00010246 [ 146.187521] RAX: > 000000000000001b RBX: ffffc9000362fc08 RCX: > 0000000000000006 > [ 146.188472] RDX: 0000000000000000 RSI: 0000000000000096 RDI: > ffff88087fc16990 > [ 146.189427] RBP: ffffc9000362fb18 R08: 00000000ffffff9d R09: > 00000000000004ac > [ 146.190392] R10: 00000000000001e7 R11: 0000000000000001 R12: > ffff88086af2e090 > [ 146.191361] R13: 0000000000000000 R14: 0000000000000001 R15: > 00000000ffffff9d > [ 146.192327] FS: 0000000000000000(0000) GS:ffff88087fc00000(0000) > knlGS:0000000000000000 > [ 146.193301] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ > 146.194274] CR2: 00000000000004a0 CR3: 000000000220a002 CR4: > 00000000003606e0 > [ 146.195258] DR0: 0000000000000000 DR1: 0000000000000000 DR2: > 0000000000000000 > [ 146.196256] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: > 0000000000000400 > [ 146.197231] Call Trace: > [ 146.198209] ? rdma_addr_register_client+0x30/0x30 [ib_core] [ > 146.199199] > rdma_resolve_ip+0x1af/0x280 [ib_core] [ 146.200196] > rdma_addr_find_l2_eth_by_grh+0x154/0x2b0 [ib_core] > > Signed-off-by: Muneendra <muneendra.kumar@xxxxxxxxxxxx> > --- > drivers/infiniband/core/addr.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/core/addr.c > b/drivers/infiniband/core/addr.c index > a5b4cf0..d61ed45 100644 > --- a/drivers/infiniband/core/addr.c > +++ b/drivers/infiniband/core/addr.c > @@ -523,7 +523,8 @@ static int addr_resolve(struct sockaddr *src_in, > ndev = dev_get_by_index(addr->net, > addr->bound_dev_if); > } else { > ndev = rt->dst.dev; > - dev_hold(ndev); > + if (ndev) > + dev_hold(ndev); Given that if addr4_resolve() or addr6_resolve() fails, rt and dst entries are not accessed, this ndev should not be NULL, do you see this NULL too? > } > > ip_rt_put(rt); > @@ -544,7 +545,8 @@ static int addr_resolve(struct sockaddr *src_in, > ndev = dev_get_by_index(addr->net, > addr->bound_dev_if); > } else { > ndev = dst->dev; > - dev_hold(ndev); > + if (ndev) > + dev_hold(ndev); > } > > dst_release(dst); > @@ -556,12 +558,15 @@ static int addr_resolve(struct sockaddr *src_in, > * Put the loopback device and get the translated > * device instead. > */ > - dev_put(ndev); > + if (ndev) > + dev_put(ndev); > ndev = dev_get_by_index(addr->net, addr->bound_dev_if); > } else { > addr->bound_dev_if = ndev->ifindex; > } > - dev_put(ndev); > + /*The ndev could be null */ > + if (ndev) > + dev_put(ndev); > > return ret; > } > -- > 1.8.3.1 > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo > info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
