netdev - Re: [ RFC ] igb: first draft of igb rtnl_link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <49CCEF29.9060200@intel.com>
Date:	Fri, 27 Mar 2009 08:22:17 -0700
From:	Alexander Duyck <alexander.h.duyck@...el.com>
To:	Patrick McHardy <kaber@...sh.net>
CC:	David Miller <davem@...emloft.net>,
	"alexander.duyck@...il.com" <alexander.duyck@...il.com>,
	"shemminger@...tta.com" <shemminger@...tta.com>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>
Subject: Re: [ RFC ] igb: first draft of igb rtnl_link_ops interface for vf
 creation

Patrick McHardy wrote:
> Alexander Duyck wrote:
>> In the meantime I have been working on the rtnl_link_ops approach and I 
>> think I have a few things going but I wanted to get some input before I 
>> go much further.
>>
>> First, is it ok for me to call rtnl_unlock prior to doing my settings 
>> changes on the sriov config space, followed by rtnl_lock afterwards in 
>> my newlink and dellink operations?  I ask because I had to do this in 
>> order to prevent a deadlock when the pci-hotplug events fired for the 
>> vfs and called unregister/register_netdev.
> 
> No, both functions are called with the RTNL already held. I'm not
> sure I understand what kind of potential deadlock you're trying
> to avoid. The ->newlink and ->dellink functions are called (mainly)
> in response to userspace netlink messages and there should never
> be a need to change anything related to rtnl locking.
> 
> A deadlock can happen when you call rtnl_link_unregister() while
> holding the RTNL. There's an unlocked version (__rtnl_link_unregister)
> for this case.
> 
> If that doesn't answer your question, please provide more detail.

So what I was seeing prior to changing the locking is that if I had the 
igbvf driver loaded and enabled a vf the operation would hang, and 
anything that tried to configure a network interface would hang as well.

The call to enable SR-IOV is contained within the newlink and dellink 
calls with this patch.  When I change the number of VFs it will trigger 
PCI hotplug events where it will remove all the VFs and then add them 
back.  As a result there are a number of register/unregister_netdev 
calls that are triggered by the igbvf_probe/remove calls in the igbvf 
driver.

> 
>> Second is it acceptable for me to just free the netdev at the end of 
>> newlink and call delete on the PF interface directly?  I ask because I 
>> don't have any use for the netdevs that are generated and I cannot call 
>> delete on specific VFs anyway since they are allocated/freed in LIFO 
>> order so I would always have to free the last one I allocated.
> 
> No, the newly created netdev is freed when returning an error, other
> netdevs should not be touched.

The problem is I have to alloc/free VFs in order.  See the rest of my 
comments on this below.

>> I have included a patch for review below that implements these changes 
>> against the current driver.  Please feel free to comment.
>>
>> +static int igb_new_vf(struct net_device *dev, struct nlattr *tb[],
>> +                      struct nlattr *data[])
>> +{
>> +    struct net_device *netdev;
>> +    struct igb_adapter *adapter;
>> +    int err;
>> +
>> +    netdev = __dev_get_by_index(dev_net(dev), nla_get_u32(tb[IFLA_LINK]));
>> +
>> +    if (!netdev)
>> +        return -ENODEV;
>> +   
>> +    adapter = netdev_priv(netdev);
>> +    err = igb_set_num_vfs(netdev, adapter->vfs_allocated_count + 1);
>> +    if (!err)
>> +        free_netdev(dev);
>> +
>> +    return err;
>> +
>> +}
>> +
>> +static void igb_del_vf(struct net_device *dev)
>> +{
>> +    struct igb_adapter *adapter = netdev_priv(dev);
>> +
>> +    if (adapter->vfs_allocated_count > 0)
>> +        igb_set_num_vfs(dev, adapter->vfs_allocated_count - 1);
> 
> Thats not really how this is supposed to work. Every device is an
> independant instance, so you can delete them in arbitrary order.
> If you need to assign them some device resources, you need to do
> this mapping internally.

This is where it gets messy and where we don't really have any good 
tools for this.  The problem is each VF is not independent.   If I 
remove VFs it has to be in LIFO ordering.  This is due to the fact that 
SR-IOV config space only allows you to specify a number of VFs, not the 
ordering of them, so they cannot be enabled/disabled individually.

Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html