From mboxrd@z Thu Jan 1 00:00:00 1970 From: Erez Shitrit Subject: Re: [PATCH linux-next 1/4] infiniband/ipoib: fix possible NULL pointer dereference in ipoib_get_iflink Date: Wed, 15 Apr 2015 09:17:14 +0300 Message-ID: <552E026A.4020200@dev.mellanox.co.il> References: <1429024817-21561-1-git-send-email-honli@redhat.com> <1429024817-21561-2-git-send-email-honli@redhat.com> <20150414204133.GJ7682@obsidianresearch.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150414204133.GJ7682-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> Sender: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jason Gunthorpe Cc: Honggang Li , Roland Dreier , sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, kaber-dcUjhNyLwpNeoWH0uzbU5w@public.gmane.org, davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org, Alex Estrin , Doug Ledford , edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org, Erez Shitrit , nicolas.dichtel-pdR9zngts4EAvxtiuMwx3w@public.gmane.org, maheshb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org, jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org, elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org, andrew-g2DYL2Zd6BY@public.gmane.org, sfeldma-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, alexander.h.duyck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, "linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-rdma@vger.kernel.org On 4/14/2015 11:41 PM, Jason Gunthorpe wrote: > On Tue, Apr 14, 2015 at 07:30:03PM +0300, Erez Shitrit wrote: > >>> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c >>> index 657b89b..11ea6e2 100644 >>> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c >>> @@ -846,7 +846,10 @@ static int ipoib_get_iflink(const struct net_device *dev) >>> { >>> struct ipoib_dev_priv *priv = netdev_priv(dev); >>> >>> - return priv->parent->ifindex; >>> + if (priv && priv->parent) >>> + return priv->parent->ifindex; >>> + else >>> + return 0; >> This will make parent interface to return 0 instead of its own ifindex. >> I would suggest write something like that: > Agree > >> + /* parent interface */ >> + if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) >> + return dev->ifindex; >> + >> + /* child/vlan interface */ >> + if (!priv->parent) >> + return -1; > Like was said for other drivers, I can't see how parent can be null > while IPOIB_FLAG_SUBINTERFACE is set. Drop the last if. It can, at least for ipoib child interface (AKA "vlan"), you can't control the call for that ndo and it can be called before the parent was set. > Erez, you basically rewrote this, please make a proper patch with the > Fixes and Reported-By credit for Honggang. Lets merge this through > Dave M's tree right away. > > Thank you all > > Jason > . > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964846AbbDOGR3 (ORCPT ); Wed, 15 Apr 2015 02:17:29 -0400 Received: from mail-wi0-f172.google.com ([209.85.212.172]:33761 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932535AbbDOGRV (ORCPT ); Wed, 15 Apr 2015 02:17:21 -0400 Message-ID: <552E026A.4020200@dev.mellanox.co.il> Date: Wed, 15 Apr 2015 09:17:14 +0300 From: Erez Shitrit User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Jason Gunthorpe CC: Honggang Li , Roland Dreier , sean.hefty@intel.com, hal.rosenstock@gmail.com, kaber@trash.net, davem@davemloft.net, Alex Estrin , Doug Ledford , edumazet@google.com, Erez Shitrit , nicolas.dichtel@6wind.com, maheshb@google.com, jbenc@redhat.com, ebiederm@xmission.com, elfring@users.sourceforge.net, f.fainelli@gmail.com, linux@roeck-us.net, andrew@lunn.ch, sfeldma@gmail.com, alexander.h.duyck@intel.com, "linux-rdma@vger.kernel.org" , linux-kernel@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [PATCH linux-next 1/4] infiniband/ipoib: fix possible NULL pointer dereference in ipoib_get_iflink References: <1429024817-21561-1-git-send-email-honli@redhat.com> <1429024817-21561-2-git-send-email-honli@redhat.com> <20150414204133.GJ7682@obsidianresearch.com> In-Reply-To: <20150414204133.GJ7682@obsidianresearch.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 4/14/2015 11:41 PM, Jason Gunthorpe wrote: > On Tue, Apr 14, 2015 at 07:30:03PM +0300, Erez Shitrit wrote: > >>> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c >>> index 657b89b..11ea6e2 100644 >>> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c >>> @@ -846,7 +846,10 @@ static int ipoib_get_iflink(const struct net_device *dev) >>> { >>> struct ipoib_dev_priv *priv = netdev_priv(dev); >>> >>> - return priv->parent->ifindex; >>> + if (priv && priv->parent) >>> + return priv->parent->ifindex; >>> + else >>> + return 0; >> This will make parent interface to return 0 instead of its own ifindex. >> I would suggest write something like that: > Agree > >> + /* parent interface */ >> + if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) >> + return dev->ifindex; >> + >> + /* child/vlan interface */ >> + if (!priv->parent) >> + return -1; > Like was said for other drivers, I can't see how parent can be null > while IPOIB_FLAG_SUBINTERFACE is set. Drop the last if. It can, at least for ipoib child interface (AKA "vlan"), you can't control the call for that ndo and it can be called before the parent was set. > Erez, you basically rewrote this, please make a proper patch with the > Fixes and Reported-By credit for Honggang. Lets merge this through > Dave M's tree right away. > > Thank you all > > Jason > . >