netdev - Re: [PATCH net-next] vlan: Invoke driver vlan hooks only if device is present

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]

Date:	Wed, 4 Nov 2015 13:31:12 +0530
From:	Padmanabh Ratnakar <padmanabh.ratnakar@...gotech.com>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH net-next] vlan: Invoke driver vlan hooks only if device is present

Hi David,

On Tue, Nov 3, 2015 at 9:46 PM, David Miller <davem@...emloft.net> wrote:
> From: Padmanabh Ratnakar <padmanabh.ratnakar@...gotech.com>
> Date: Tue,  3 Nov 2015 20:25:59 +0530
>
>> NIC drivers mark device as detached during error recovery.
>> It expects no manangement hooks to be invoked in this state.
>> Invoke driver vlan hooks only if device is present.
>>
>> Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@...gotech.com>
>
> I don't like this.
>
> This approach to solving the issue just peppers checks randomly around
> the kernel and the risk for missing cases is very high.  It is also
> not clear to me what exactly specifies that these calls are not valid
> in such states.
>
> Traditionally we've depended upon the driver to make this kind of check
> in it's callback, and that way we don't need to add extra overhead for
> everyone at all of these method invokcation points.
>
> Therefore, I would like to see a less dirty, and less error prone
> approach to solving this bug.

I am trying to figure out the correct place for this check.

Currently for invoking of most managements hooks, kernel follows below pattern -
(ethtool, set mac, set mtu etc..)

rtlnl_lock();
if (!netif_device_present(netdev))
        return -ENODEV;
if (ops->ndo_xxxxx)
         ops->ndo_xxxxx;
rtnl_unlock();

Many drivers((emulex, intel, tg3 etc)) when they are recovering from error like
PCIe error, do the following -

In error_detected routine, before starting recovery -

rtnl_lock();
...
netif_device_detach(netdev);
....
rtnl_unlock();

After error is recovered, driver moves the device to attached state
using netif_device_attach().
Drivers use other methods to quiesce data path hooks.

Management hooks implementation of driver don't check if device is in
error recovery.
They expect that these hooks wont get called if device is marked as not present.

Device present check is missing for vlan addition/deletion.
Be2net driver may have an issue if this hook gets invoked during error recovery.
I thought it may be better if this is also handled like other management hooks.
Please let me know if I am missing anything and what you feel in right
place to put this
check - in kernel or in driver.


Thanks,
Padmanabh
--
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