The four callback functions deferference the pointers without test.
As ipoib_get_iflink failed, it is a good reason to fix the other three
functions. Those functions will return zero on error, as valid ifindex
greater than zero.
I understand that those simple patches should be emerged into a single
patch. However, those files have be maintained by different maintainers.
So, I split it for review. It also will be convenient to remove one or
more patches with the rest not be affected.
Honggang Li (4):
infiniband/ipoib: fix possible NULL pointer dereference in
ipoib_get_iflink
ipvlan: fix possible NULL pointer dereference in ipvlan_get_iflink
macvlan: fix possible NULL pointer dereference in macvlan_dev_get_iflink
net/dsa: fix possible NULL pointer dereference in dsa_slave_get_iflink
drivers/infiniband/ulp/ipoib/ipoib_main.c | 5 ++++-
drivers/net/ipvlan/ipvlan_main.c | 5 ++++-
drivers/net/macvlan.c | 4 +++-
net/dsa/slave.c | 5 ++++-
4 files changed, 15 insertions(+), 4 deletions(-)
--
1.8.3.1
Starting monitoring for VG vg_rdma01: 3 logical volume(s) in volume
group "vg_rdma01" monitored
[ OK ]
Starting cgconfig service: Failed to parse /etc/cgconfig.conf or
/etc/cgconfig.d[FAILED]
Loading OpenIB kernel modules:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000120
IP: [<ffffffffa06b9060>] ipoib_get_iflink+0x10/0x20 [ib_ipoib]
PGD 475540067 PUD 473541067 PMD 0
Oops: 0000 [#1] SMP
Modules linked in: ib_ipoib(+) rdma_ucm ib_ucm ib_uverbs ib_umad rdma_cm
ib_cm ib_sa vhost_net macvtap macvlan vhost tun ipmi_devintf sg ipmi_si
ipmi_msghandler serio_raw iTCO_wdt iTCO_vendor_support cdc_ether usbnet
mii bnx2 intel_powerclamp coretemp kvm_intel kvm crc32c_intel
ghash_clmulni_intel aesni_intel ablk_helper cryptd lrw gf128mul
glue_helper aes_x86_64 microcode pcspkr i2c_i801 i2c_core lpc_ich
mfd_core acpi_cpufreq ioatdma i7core_edac edac_core shpchp ext4(E)
jbd2(E) mbcache(E) sd_mod(E) megaraid_sas(E) pata_acpi(E) ata_generic(E)
ata_piix(E) iw_cxgb3(E) cxgb3(E) mdio(E) ib_qib(E) dca(E) ib_mad(E)
iw_cxgb4(E) iw_cm(E) ib_core(E) ib_addr(E) ipv6(E) cxgb4(E) dm_mirror(E)
dm_region_hash(E) dm_log(E) dm_mod(E)
CPU: 6 PID: 2405 Comm: modprobe Tainted: G E
4.0.0-next-20150413 #1
Hardware name: IBM System x3650 M3 -[7945O63]-/00D4062, BIOS
-[D6E157AUS-1.15]- 06/13/2012
task: ffff880476ad6f00 ti: ffff88047579c000 task.ti: ffff88047579c000
RIP: 0010:[<ffffffffa06b9060>] [<ffffffffa06b9060>]
ipoib_get_iflink+0x10/0x20 [ib_ipoib]
RSP: 0018:ffff88047579f9b8 EFLAGS: 00010286
RAX: 0000000000000000 RBX: ffff880476e2a000 RCX: 0000000000000000
RDX: 0000000000000004 RSI: ffff88047579fbb8 RDI: ffff880476e2a000
RBP: ffff88047579f9b8 R08: 0000000000000660 R09: ffff88047404f068
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8804736bec00
R13: ffff88047579fbb4 R14: ffff88047404f000 R15: 0000000000000009
FS: 00007fc047a2e700(0000) GS:ffff88047fc00000(0000)
knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000120 CR3: 000000047541f000 CR4: 00000000000006e0
Stack:
ffff88047579f9c8 ffffffff814fbfa3 ffff88047579fbe8 ffffffff81515a15
0000000000000005 ffff880476e2a280 0000000000000005 0000000000000014
ffff88047579fa48 ffffffff8150a577 0000000000000000 ffff8804ffffffff
Call Trace:
[<ffffffff814fbfa3>] dev_get_iflink+0x23/0x40
[<ffffffff81515a15>] rtnl_fill_ifinfo+0x255/0xce0
[<ffffffff8150a577>] ? __hw_addr_create_ex+0x97/0xc0
[<ffffffff815d32bb>] ? _raw_spin_unlock_bh+0x1b/0x20
[<ffffffff8150a8e5>] ? __dev_mc_add+0x75/0x90
[<ffffffffa00a115c>] ? igmp6_group_added+0x5c/0x130 [ipv6]
[<ffffffff8119c6cc>] ? __kmalloc_node_track_caller+0x3c/0x50
[<ffffffff814f0f0b>] ? __kmalloc_reserve+0x3b/0xa0
[<ffffffff814f12f8>] ? __alloc_skb+0xa8/0x1f0
[<ffffffff81516783>] rtmsg_ifinfo_build_skb+0x83/0xe0
[<ffffffff81078fa6>] ? raw_notifier_call_chain+0x16/0x20
[<ffffffff81516801>] rtmsg_ifinfo+0x21/0x40
[<ffffffff81504eaf>] register_netdevice+0x38f/0x400
[<ffffffff81504f3e>] register_netdev+0x1e/0x30
[<ffffffffa06bc204>] ipoib_add_port.clone.0+0x214/0x390 [ib_ipoib]
[<ffffffffa06bc447>] ipoib_add_one+0xc7/0x110 [ib_ipoib]
[<ffffffffa00f9d4d>] ib_register_client+0x7d/0xa0 [ib_core]
[<ffffffffa06ce000>] ? 0xffffffffa06ce000
[<ffffffffa06ce0f2>] ipoib_init_module+0xf2/0x13c [ib_ipoib]
[<ffffffff81000287>] do_one_initcall+0xb7/0x1d0
[<ffffffff810d8189>] do_init_module+0x69/0x200
[<ffffffff810da985>] load_module+0x5b5/0x730
[<ffffffff810d79b0>] ? mod_sysfs_teardown+0x150/0x150
[<ffffffff81183232>] ? __vmalloc+0x22/0x30
[<ffffffff810d73c0>] ? module_sect_show+0x30/0x30
[<ffffffff810dac84>] SyS_init_module+0x94/0xc0
[<ffffffff815d3997>] system_call_fastpath+0x12/0x6a
Code: 66 66 66 90 b9 1e 00 00 00 48 89 f0 48 8d 77 08 48 89 c7 f3 48 a5
c9 c3 0f 1f 00 55 48 89 e5 66 66 66 66 90 48 8b 87 e8 13 00 00 <8b> 80
20 01 00 00 c9 c3 0f 1f 84 00 00 00 00 00 55 48 89 e5 66
RIP [<ffffffffa06b9060>] ipoib_get_iflink+0x10/0x20 [ib_ipoib]
RSP <ffff88047579f9b8>
CR2: 0000000000000120
---[ end trace a8610f6e9640eb85 ]---
Signed-off-by: Honggang Li <[email protected]>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 657b89b..11ea6e2 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ 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;
}
static u32 ipoib_addr_hash(struct ipoib_neigh_hash *htbl, u8 *daddr)
--
1.8.3.1
Signed-off-by: Honggang Li <[email protected]>
---
drivers/net/ipvlan/ipvlan_main.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 77b92a0..479fcf7 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -308,7 +308,10 @@ static int ipvlan_get_iflink(const struct net_device *dev)
{
struct ipvl_dev *ipvlan = netdev_priv(dev);
- return ipvlan->phy_dev->ifindex;
+ if (ipvlan && pvlan->phy_dev)
+ return ipvlan->phy_dev->ifindex;
+ else
+ return 0;
}
static const struct net_device_ops ipvlan_netdev_ops = {
--
1.8.3.1
Signed-off-by: Honggang Li <[email protected]>
---
drivers/net/macvlan.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index b227a13..1e59f39 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -998,7 +998,9 @@ static int macvlan_dev_get_iflink(const struct net_device *dev)
{
struct macvlan_dev *vlan = netdev_priv(dev);
- return vlan->lowerdev->ifindex;
+ if (vlan && vlan->lowerdev)
+ return vlan->lowerdev->ifindex;
+ return 0;
}
static const struct ethtool_ops macvlan_ethtool_ops = {
--
1.8.3.1
Signed-off-by: Honggang Li <[email protected]>
---
net/dsa/slave.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 827cda56..070b599 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -59,7 +59,10 @@ static int dsa_slave_get_iflink(const struct net_device *dev)
{
struct dsa_slave_priv *p = netdev_priv(dev);
- return p->parent->dst->master_netdev->ifindex;
+ if (p && p->parent && p->parent->dst && p->parent->dst->master_netdev)
+ return p->parent->dst->master_netdev->ifindex;
+ else
+ return 0;
}
static inline bool dsa_port_is_bridged(struct dsa_slave_priv *p)
--
1.8.3.1
On 14.04, Honggang Li wrote:
> Signed-off-by: Honggang Li <[email protected]>
> ---
> drivers/net/macvlan.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index b227a13..1e59f39 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -998,7 +998,9 @@ static int macvlan_dev_get_iflink(const struct net_device *dev)
> {
> struct macvlan_dev *vlan = netdev_priv(dev);
>
> - return vlan->lowerdev->ifindex;
> + if (vlan && vlan->lowerdev)
> + return vlan->lowerdev->ifindex;
That is completely useless. vlan (=netdev_priv) can not be NULL as
netdev_priv() never returns NULL and vlan->lowerdev is always valid
because a macvlan wouldn't make much sense otherwise.
On Tue, Apr 14, 2015 at 04:26:27PM +0100, Patrick McHardy wrote:
>
> That is completely useless. vlan (=netdev_priv) can not be NULL as
> netdev_priv() never returns NULL and vlan->lowerdev is always valid
> because a macvlan wouldn't make much sense otherwise.
OK, please drop this patch.
thanks
On Tue, 2015-04-14 at 23:20 +0800, Honggang Li wrote:
> Starting monitoring for VG vg_rdma01: 3 logical volume(s) in volume
> group "vg_rdma01" monitored
> [ OK ]
> CR2: 0000000000000120
> ---[ end trace a8610f6e9640eb85 ]---
>
> Signed-off-by: Honggang Li <[email protected]>
When was this bug added ?
Please add a proper Fixes: tag
Le 14/04/2015 17:26, Patrick McHardy a ?crit :
> On 14.04, Honggang Li wrote:
[snip]
>> - return vlan->lowerdev->ifindex;
>> + if (vlan && vlan->lowerdev)
>> + return vlan->lowerdev->ifindex;
>
> That is completely useless. vlan (=netdev_priv) can not be NULL as
> netdev_priv() never returns NULL and vlan->lowerdev is always valid
> because a macvlan wouldn't make much sense otherwise.
>
And I suspect that it is the same for ipvlan and dsa.
On 14.04, Honggang LI wrote:
> On Tue, Apr 14, 2015 at 04:26:27PM +0100, Patrick McHardy wrote:
> >
> > That is completely useless. vlan (=netdev_priv) can not be NULL as
> > netdev_priv() never returns NULL and vlan->lowerdev is always valid
> > because a macvlan wouldn't make much sense otherwise.
>
> OK, please drop this patch.
Well, the fact that netdev_priv never return NULL implies all your
patches need to be redone.
And I suggest you check whether this condition can actually happen.
Just because it can in one driver says nothing at all about others.
On Tue, Apr 14, 2015 at 05:35:12PM +0200, Nicolas Dichtel wrote:
> Le 14/04/2015 17:26, Patrick McHardy a ?crit :
> >On 14.04, Honggang Li wrote:
> [snip]
> >>- return vlan->lowerdev->ifindex;
> >>+ if (vlan && vlan->lowerdev)
> >>+ return vlan->lowerdev->ifindex;
> >
> >That is completely useless. vlan (=netdev_priv) can not be NULL as
> >netdev_priv() never returns NULL and vlan->lowerdev is always valid
> >because a macvlan wouldn't make much sense otherwise.
> >
> And I suspect that it is the same for ipvlan and dsa.
I agree about DSA. I don't see any way this could happen.
Andrew
On Tue, Apr 14, 2015 at 08:34:33AM -0700, Eric Dumazet wrote:
> On Tue, 2015-04-14 at 23:20 +0800, Honggang Li wrote:
> > Starting monitoring for VG vg_rdma01: 3 logical volume(s) in volume
> > group "vg_rdma01" monitored
> > [ OK ]
>
>
> > CR2: 0000000000000120
> > ---[ end trace a8610f6e9640eb85 ]---
> >
> > Signed-off-by: Honggang Li <[email protected]>
>
> When was this bug added ?
>
Sorry, I do not know. I ran into this bug today when I'm tracing a race
condition issue related qib. In order to check upstream linux-next fix
the race condition or not, I build linux-next-20150414 on two machines. Both
machines panic at modprobe ib_ipoib. Do you means I need to report a
bug? But I do not know report to who or where.
thanks
> Please add a proper Fixes: tag
>
>
On Tue, Apr 14, 2015 at 05:37:57PM +0200, Andrew Lunn wrote:
> > >
> > And I suspect that it is the same for ipvlan and dsa.
>
> I agree about DSA. I don't see any way this could happen.
>
> Andrew
I only keep the ipoib patch and drop the rest patches.
thanks
Le 14/04/2015 17:44, Honggang LI a ?crit :
> On Tue, Apr 14, 2015 at 08:34:33AM -0700, Eric Dumazet wrote:
>> On Tue, 2015-04-14 at 23:20 +0800, Honggang Li wrote:
>>> Starting monitoring for VG vg_rdma01: 3 logical volume(s) in volume
>>> group "vg_rdma01" monitored
>>> [ OK ]
>>
>>
>>> CR2: 0000000000000120
>>> ---[ end trace a8610f6e9640eb85 ]---
>>>
>>> Signed-off-by: Honggang Li <[email protected]>
>>
>> When was this bug added ?
>>
>
> Sorry, I do not know. I ran into this bug today when I'm tracing a race
> condition issue related qib. In order to check upstream linux-next fix
> the race condition or not, I build linux-next-20150414 on two machines. Both
> machines panic at modprobe ib_ipoib. Do you means I need to report a
> bug? But I do not know report to who or where.
Here is the tag:
Fixes: 5aa7add8f14b ("infiniband/ipoib: implement ndo_get_iflink")
On Tue, Apr 14, 2015 at 05:49:55PM +0200, Nicolas Dichtel wrote:
> Le 14/04/2015 17:44, Honggang LI a écrit :
> >On Tue, Apr 14, 2015 at 08:34:33AM -0700, Eric Dumazet wrote:
> >>On Tue, 2015-04-14 at 23:20 +0800, Honggang Li wrote:
> >>>Starting monitoring for VG vg_rdma01: 3 logical volume(s) in volume
> >>>group "vg_rdma01" monitored
> >>>[ OK ]
> >>
> >>
> >>>CR2: 0000000000000120
> >>>---[ end trace a8610f6e9640eb85 ]---
> >>>
> >>>Signed-off-by: Honggang Li <[email protected]>
> >>
> >>When was this bug added ?
> >>
> >
> >Sorry, I do not know. I ran into this bug today when I'm tracing a race
> >condition issue related qib. In order to check upstream linux-next fix
> >the race condition or not, I build linux-next-20150414 on two machines. Both
> >machines panic at modprobe ib_ipoib. Do you means I need to report a
> >bug? But I do not know report to who or where.
>
> Here is the tag:
> Fixes: 5aa7add8f14b ("infiniband/ipoib: implement ndo_get_iflink")
>
thank you.
On Tue, Apr 14, 2015 at 11:20:17PM +0800, Honggang Li wrote:
> Signed-off-by: Honggang Li <[email protected]>
> ---
> net/dsa/slave.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 827cda56..070b599 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -59,7 +59,10 @@ static int dsa_slave_get_iflink(const struct net_device *dev)
> {
> struct dsa_slave_priv *p = netdev_priv(dev);
>
> - return p->parent->dst->master_netdev->ifindex;
> + if (p && p->parent && p->parent->dst && p->parent->dst->master_netdev)
> + return p->parent->dst->master_netdev->ifindex;
> + else
> + return 0;
> }
I would expect some explanation why you believe that any of those pointers
can be NULL.
Guenter
Hi Nicolas,
Le mardi 14 avril 2015 à 17:49 +0200, Nicolas Dichtel a écrit :
> Le 14/04/2015 17:44, Honggang LI a écrit :
> > On Tue, Apr 14, 2015 at 08:34:33AM -0700, Eric Dumazet wrote:
> >> On Tue, 2015-04-14 at 23:20 +0800, Honggang Li wrote:
> >>> Starting monitoring for VG vg_rdma01: 3 logical volume(s) in volume
> >>> group "vg_rdma01" monitored
> >>> [ OK ]
> >>
> >>
> >>> CR2: 0000000000000120
> >>> ---[ end trace a8610f6e9640eb85 ]---
> >>>
> >>> Signed-off-by: Honggang Li <[email protected]>
> >>
> >> When was this bug added ?
> >>
> >
> > Sorry, I do not know. I ran into this bug today when I'm tracing a race
> > condition issue related qib. In order to check upstream linux-next fix
> > the race condition or not, I build linux-next-20150414 on two machines. Both
> > machines panic at modprobe ib_ipoib. Do you means I need to report a
> > bug? But I do not know report to who or where.
>
> Here is the tag:
> Fixes: 5aa7add8f14b ("infiniband/ipoib: implement ndo_get_iflink")
>
Pardon me, but this patch was never submitted to
[email protected] for review !?
Regards.
--
Yann Droneaud
OPTEYA
On Tue, 2015-04-14 at 23:53 +0800, Honggang LI wrote:
> On Tue, Apr 14, 2015 at 05:49:55PM +0200, Nicolas Dichtel wrote:
> > Le 14/04/2015 17:44, Honggang LI a écrit :
> > >On Tue, Apr 14, 2015 at 08:34:33AM -0700, Eric Dumazet wrote:
> > >>On Tue, 2015-04-14 at 23:20 +0800, Honggang Li wrote:
> > >>>Starting monitoring for VG vg_rdma01: 3 logical volume(s) in volume
> > >>>group "vg_rdma01" monitored
> > >>>[ OK ]
> > >>
> > >>
> > >>>CR2: 0000000000000120
> > >>>---[ end trace a8610f6e9640eb85 ]---
> > >>>
> > >>>Signed-off-by: Honggang Li <[email protected]>
> > >>
> > >>When was this bug added ?
> > >>
> > >
> > >Sorry, I do not know. I ran into this bug today when I'm tracing a race
> > >condition issue related qib. In order to check upstream linux-next fix
> > >the race condition or not, I build linux-next-20150414 on two machines. Both
> > >machines panic at modprobe ib_ipoib. Do you means I need to report a
> > >bug? But I do not know report to who or where.
> >
> > Here is the tag:
> > Fixes: 5aa7add8f14b ("infiniband/ipoib: implement ndo_get_iflink")
> >
>
> thank you.
By adding a proper tag you :
1) Assert you did a research in git history/blame to find original
commit.
2) You looked at other similar bugs added by this commit
3) Add meta information in git history to ease backports
4) CC original author to let him/her know he made a mistake and increase
his/her knowledge.
5) Help maintainers
1) Droped ipvlan/macvlan/dsa patches.
2) Do not check the pointer returned by netdev_priv.
3) Add the FIX tag.
--------------------- console log ---------------
ipoib_get_iflink dev = ffff88026eb1c000, priv = ffff88026eb1c7c0, priv->parent = (null)
ipoib_get_iflink dev = ffff88026eb1c000, priv = ffff88026eb1c7c0, priv->parent = (null)
ipoib_get_iflink dev = ffff880225c8a000, priv = ffff880225c8a7c0, priv->parent = (null)
ipoib_get_iflink dev = ffff880225c8a000, priv = ffff880225c8a7c0, priv->parent = (null)
[ OK ]
ipoib_get_iflink dev = ffff88026eb1c000, priv = ffff88026eb1c7c0, priv->parent = (null)
ipoib_get_iflink dev = ffff88026eb1c000, priv = ffff88026eb1c7c0, priv->parent = (null)
ipoib_get_iflink dev = ffff880225c8a000, priv = ffff880225c8a7c0, priv->parent = (null)
ipoib_get_iflink dev = ffff880225c8a000, priv = ffff880225c8a7c0, priv->parent = (null)
Shutting down interface cxgb4_1: ipoib_get_iflink dev = ffff88026eb1c000, priv = ffff88026eb1c7c0, priv->parent = (null)
ipoib_get_iflink dev = ffff88026eb1c000, priv = ffff88026eb1c7c0, priv->parent = (null)
ipoib_get_iflink dev = ffff880225c8a000, priv = ffff880225c8a7c0, priv->parent = (null)
ipoib_get_iflink dev = ffff880225c8a000, priv = ffff880225c8a7c0, priv->parent = (null)
--------------------- console log ---------------
Honggang Li (1):
infiniband/ipoib: fix possible NULL pointer dereference in
ipoib_get_iflink
drivers/infiniband/ulp/ipoib/ipoib_main.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
--
1.8.3.1
Starting monitoring for VG vg_rdma01: 3 logical volume(s) in volume
group "vg_rdma01" monitored
[ OK ]
Starting cgconfig service: Failed to parse /etc/cgconfig.conf or
/etc/cgconfig.d[FAILED]
Loading OpenIB kernel modules:
BUG: unable to handle kernel NULL pointer dereference at
0000000000000120
IP: [<ffffffffa06b9060>] ipoib_get_iflink+0x10/0x20 [ib_ipoib]
PGD 475540067 PUD 473541067 PMD 0
Oops: 0000 [#1] SMP
Modules linked in: ib_ipoib(+) rdma_ucm ib_ucm ib_uverbs ib_umad rdma_cm
ib_cm ib_sa vhost_net macvtap macvlan vhost tun ipmi_devintf sg ipmi_si
ipmi_msghandler serio_raw iTCO_wdt iTCO_vendor_support cdc_ether usbnet
mii bnx2 intel_powerclamp coretemp kvm_intel kvm crc32c_intel
ghash_clmulni_intel aesni_intel ablk_helper cryptd lrw gf128mul
glue_helper aes_x86_64 microcode pcspkr i2c_i801 i2c_core lpc_ich
mfd_core acpi_cpufreq ioatdma i7core_edac edac_core shpchp ext4(E)
jbd2(E) mbcache(E) sd_mod(E) megaraid_sas(E) pata_acpi(E) ata_generic(E)
ata_piix(E) iw_cxgb3(E) cxgb3(E) mdio(E) ib_qib(E) dca(E) ib_mad(E)
iw_cxgb4(E) iw_cm(E) ib_core(E) ib_addr(E) ipv6(E) cxgb4(E) dm_mirror(E)
dm_region_hash(E) dm_log(E) dm_mod(E)
CPU: 6 PID: 2405 Comm: modprobe Tainted: G E
4.0.0-next-20150413 #1
Hardware name: IBM System x3650 M3 -[7945O63]-/00D4062, BIOS
-[D6E157AUS-1.15]- 06/13/2012
task: ffff880476ad6f00 ti: ffff88047579c000 task.ti: ffff88047579c000
RIP: 0010:[<ffffffffa06b9060>] [<ffffffffa06b9060>]
ipoib_get_iflink+0x10/0x20 [ib_ipoib]
RSP: 0018:ffff88047579f9b8 EFLAGS: 00010286
RAX: 0000000000000000 RBX: ffff880476e2a000 RCX: 0000000000000000
RDX: 0000000000000004 RSI: ffff88047579fbb8 RDI: ffff880476e2a000
RBP: ffff88047579f9b8 R08: 0000000000000660 R09: ffff88047404f068
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8804736bec00
R13: ffff88047579fbb4 R14: ffff88047404f000 R15: 0000000000000009
FS: 00007fc047a2e700(0000) GS:ffff88047fc00000(0000)
knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000120 CR3: 000000047541f000 CR4: 00000000000006e0
Stack:
ffff88047579f9c8 ffffffff814fbfa3 ffff88047579fbe8 ffffffff81515a15
0000000000000005 ffff880476e2a280 0000000000000005 0000000000000014
ffff88047579fa48 ffffffff8150a577 0000000000000000 ffff8804ffffffff
Call Trace:
[<ffffffff814fbfa3>] dev_get_iflink+0x23/0x40
[<ffffffff81515a15>] rtnl_fill_ifinfo+0x255/0xce0
[<ffffffff8150a577>] ? __hw_addr_create_ex+0x97/0xc0
[<ffffffff815d32bb>] ? _raw_spin_unlock_bh+0x1b/0x20
[<ffffffff8150a8e5>] ? __dev_mc_add+0x75/0x90
[<ffffffffa00a115c>] ? igmp6_group_added+0x5c/0x130 [ipv6]
[<ffffffff8119c6cc>] ? __kmalloc_node_track_caller+0x3c/0x50
[<ffffffff814f0f0b>] ? __kmalloc_reserve+0x3b/0xa0
[<ffffffff814f12f8>] ? __alloc_skb+0xa8/0x1f0
[<ffffffff81516783>] rtmsg_ifinfo_build_skb+0x83/0xe0
[<ffffffff81078fa6>] ? raw_notifier_call_chain+0x16/0x20
[<ffffffff81516801>] rtmsg_ifinfo+0x21/0x40
[<ffffffff81504eaf>] register_netdevice+0x38f/0x400
[<ffffffff81504f3e>] register_netdev+0x1e/0x30
[<ffffffffa06bc204>] ipoib_add_port.clone.0+0x214/0x390 [ib_ipoib]
[<ffffffffa06bc447>] ipoib_add_one+0xc7/0x110 [ib_ipoib]
[<ffffffffa00f9d4d>] ib_register_client+0x7d/0xa0 [ib_core]
[<ffffffffa06ce000>] ? 0xffffffffa06ce000
[<ffffffffa06ce0f2>] ipoib_init_module+0xf2/0x13c [ib_ipoib]
[<ffffffff81000287>] do_one_initcall+0xb7/0x1d0
[<ffffffff810d8189>] do_init_module+0x69/0x200
[<ffffffff810da985>] load_module+0x5b5/0x730
[<ffffffff810d79b0>] ? mod_sysfs_teardown+0x150/0x150
[<ffffffff81183232>] ? __vmalloc+0x22/0x30
[<ffffffff810d73c0>] ? module_sect_show+0x30/0x30
[<ffffffff810dac84>] SyS_init_module+0x94/0xc0
[<ffffffff815d3997>] system_call_fastpath+0x12/0x6a
Code: 66 66 66 90 b9 1e 00 00 00 48 89 f0 48 8d 77 08 48 89 c7 f3 48 a5
c9 c3 0f 1f 00 55 48 89 e5 66 66 66 66 90 48 8b 87 e8 13 00 00 <8b> 80
20 01 00 00 c9 c3 0f 1f 84 00 00 00 00 00 55 48 89 e5 66
RIP [<ffffffffa06b9060>] ipoib_get_iflink+0x10/0x20 [ib_ipoib]
RSP <ffff88047579f9b8>
CR2: 0000000000000120
---[ end trace a8610f6e9640eb85 ]---
Fixes: 5aa7add8f14b ("infiniband/ipoib: implement ndo_get_iflink")
Signed-off-by: Honggang Li <[email protected]>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 657b89b..fb2a9df 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ 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->parent)
+ return priv->parent->ifindex;
+ else
+ return 0;
}
static u32 ipoib_addr_hash(struct ipoib_neigh_hash *htbl, u8 *daddr)
--
1.8.3.1
On Tue, Apr 14, 2015 at 6:20 PM, Honggang Li <[email protected]> wrote:
>
[...]
Hi,
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 657b89b..11ea6e2 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ 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:
+ /* parent interface */
+ if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags))
+ return dev->ifindex;
+
+ /* child/vlan interface */
+ if (!priv->parent)
+ return -1;
+
return priv->parent->ifindex;
Thanks,
Erez.
> }
>
> static u32 ipoib_addr_hash(struct ipoib_neigh_hash *htbl, u8 *daddr)
> --
> 1.8.3.1
>
Le 14/04/2015 18:01, Yann Droneaud a écrit :
[snip]
>> Here is the tag:
>> Fixes: 5aa7add8f14b ("infiniband/ipoib: implement ndo_get_iflink")
>>
>
> Pardon me, but this patch was never submitted to
> [email protected] for review !?
Sorry for that, I missed it. Only Roland Dreier was CCed.
Le 14/04/2015 18:30, Erez Shitrit a écrit :
> On Tue, Apr 14, 2015 at 6:20 PM, Honggang Li <[email protected]> wrote:
[snip]
> This will make parent interface to return 0 instead of its own ifindex.
> I would suggest write something like that:
>
> + /* parent interface */
> + if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags))
> + return dev->ifindex;
> +
> + /* child/vlan interface */
> + if (!priv->parent)
> + return -1;
'return 0' here.
From: Honggang LI <[email protected]>
Date: Tue, 14 Apr 2015 23:32:39 +0800
> On Tue, Apr 14, 2015 at 04:26:27PM +0100, Patrick McHardy wrote:
>>
>> That is completely useless. vlan (=netdev_priv) can not be NULL as
>> netdev_priv() never returns NULL and vlan->lowerdev is always valid
>> because a macvlan wouldn't make much sense otherwise.
>
> OK, please drop this patch.
That's not how this works.
When a patch series needs any chnages, you must make a fresh, new
complete submission of the entire patch series.
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.
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
On Tue, Apr 14, 2015 at 07:30:03PM +0300, Erez Shitrit wrote:
> > @@ -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:
>
> + /* parent interface */
> + if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags))
> + return dev->ifindex;
> +
Hi, Erez
Sorry for delay of reply. It was about 01:00 in the morning, so I
went into bed. And thank you for the suggestion. You are right. After
insert some printk statements in the driver, I confirmed it.
---------------------- console log --------------------
ipoib_get_iflink: priv = ffff880275e487c0, priv->parent = (null), priv->flags = 0x20f, dev = ffff880275e48000, dev->name = qib_ib1
qib_ib1, test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags) = 0
qib_ib1, idev->ifindex = 14
ipoib_get_iflink: priv = ffff8802765d27c0, priv->parent = (null), priv->flags = 0x20f, dev = ffff8802765d2000, dev->name = qib_ib2
qib_ib2, test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags) = 0
qib_ib2, idev->ifindex = 15
ipoib_get_iflink: priv = ffff8804741a47c0, priv->parent = ffff880275e48000, priv->flags = 0x224, dev = ffff8804741a4000, dev->name = qib_ib1.8003
qib_ib1.8003, test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags) = 1
qib_ib1.8003, idev->ifindex = 16
---------------------- console log --------------------
I will rewrite the patch.
> + /* child/vlan interface */
> + if (!priv->parent)
> + return -1;
> +
> return priv->parent->ifindex;
>
> Thanks,
> Erez.
>
> > }
> >
> > static u32 ipoib_addr_hash(struct ipoib_neigh_hash *htbl, u8 *daddr)
> > --
> > 1.8.3.1
> >
On 4/14/2015 11:41 PM, Jason Gunthorpe wrote:
> 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.
Agree, Erez, add proper Fixes: XXX note and send a patch to netdev
against net-next. No need for the lengthy crash dump there. Nicolas,
next time you patch IPoIB, please cc linux-rdma.
Or.
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
> .
>
There was network accident in the office. I can't find this email in
public mailing list. So, I reset it. If you had recived this, please
ignore it.
thanks
On Wed, Apr 15, 2015 at 01:16:40PM +0800, Honggang LI wrote:
> On Tue, Apr 14, 2015 at 07:30:03PM +0300, Erez Shitrit wrote:
> > > @@ -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:
> >
> > + /* parent interface */
> > + if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags))
> > + return dev->ifindex;
> > +
>
> Hi, Erez
>
> Sorry for delay of reply. It was about 01:00 in the morning, so I
> went into bed. And thank you for the suggestion. You are right. After
> insert some printk statements in the driver, I confirmed it.
>
> ---------------------- console log --------------------
> ipoib_get_iflink: priv = ffff880275e487c0, priv->parent = (null), priv->flags = 0x20f, dev = ffff880275e48000, dev->name = qib_ib1
> qib_ib1, test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags) = 0
> qib_ib1, idev->ifindex = 14
>
> ipoib_get_iflink: priv = ffff8802765d27c0, priv->parent = (null), priv->flags = 0x20f, dev = ffff8802765d2000, dev->name = qib_ib2
> qib_ib2, test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags) = 0
> qib_ib2, idev->ifindex = 15
>
> ipoib_get_iflink: priv = ffff8804741a47c0, priv->parent = ffff880275e48000, priv->flags = 0x224, dev = ffff8804741a4000, dev->name = qib_ib1.8003
> qib_ib1.8003, test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags) = 1
> qib_ib1.8003, idev->ifindex = 16
>
> ---------------------- console log --------------------
>
> I will rewrite the patch.
>
> > + /* child/vlan interface */
> > + if (!priv->parent)
> > + return -1;
> > +
> > return priv->parent->ifindex;
> >
> > Thanks,
> > Erez.
> >
> > > }
> > >
> > > static u32 ipoib_addr_hash(struct ipoib_neigh_hash *htbl, u8 *daddr)
> > > --
> > > 1.8.3.1
> > >
On Wed, Apr 15, 2015 at 09:17:14AM +0300, Erez Shitrit wrote:
> >>+ /* 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.
If the ndo can be called before the netdev private structures are fully
prepared then we have another bug, and returning -1 or 0 is not the right
answer anyhow.
For safety, fold this into your patch.
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index 9fad7b5ac8b9..e62b007adf5d 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -58,6 +58,7 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
/* MTU will be reset when mcast join happens */
priv->dev->mtu = IPOIB_UD_MTU(priv->max_ib_mtu);
priv->mcast_mtu = priv->admin_mtu = priv->dev->mtu;
+ priv->parent = ppriv->dev;
set_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags);
result = ipoib_set_dev_features(priv, ppriv->ca);
@@ -84,8 +85,6 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
goto register_failed;
}
- priv->parent = ppriv->dev;
-
ipoib_create_debug_files(priv->dev);
/* RTNL childs don't need proprietary sysfs entries */
On Wed, Apr 15, 2015 at 7:06 PM, Jason Gunthorpe
<[email protected]> wrote:
> On Wed, Apr 15, 2015 at 09:17:14AM +0300, Erez Shitrit wrote:
>> >>+ /* 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.
>
> If the ndo can be called before the netdev private structures are fully
> prepared then we have another bug, and returning -1 or 0 is not the right
> answer anyhow.
>
> For safety, fold this into your patch.
OK, will do that.
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> index 9fad7b5ac8b9..e62b007adf5d 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> @@ -58,6 +58,7 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
> /* MTU will be reset when mcast join happens */
> priv->dev->mtu = IPOIB_UD_MTU(priv->max_ib_mtu);
> priv->mcast_mtu = priv->admin_mtu = priv->dev->mtu;
> + priv->parent = ppriv->dev;
> set_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags);
>
> result = ipoib_set_dev_features(priv, ppriv->ca);
> @@ -84,8 +85,6 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
> goto register_failed;
> }
>
> - priv->parent = ppriv->dev;
> -
> ipoib_create_debug_files(priv->dev);
>
> /* RTNL childs don't need proprietary sysfs entries */