linux-kernel - Re: [PATCH v6 net-next 17/17] net: qualcomm: add QCA7000 UART driver

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 23 May 2017 23:01:01 +0200
From:   Lino Sanfilippo <LinoSanfilippo@....de>
To:     Stefan Wahren <stefan.wahren@...e.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "David S. Miller" <davem@...emloft.net>
Cc:     linux-serial@...r.kernel.org, Jiri Slaby <jslaby@...e.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jakub Kicinski <kubakici@...pl>, devicetree@...r.kernel.org
Subject: Re: [PATCH v6 net-next 17/17] net: qualcomm: add QCA7000 UART driver

On 23.05.2017 21:38, Stefan Wahren wrote:
> 
>> Lino Sanfilippo <LinoSanfilippo@....de> hat am 23. Mai 2017 um 20:16 geschrieben:
>>
>>
>> Hi,
>>
>> On 23.05.2017 15:12, Stefan Wahren wrote:
>>
>>
>>> +}
>>> +
>>> +static void qca_uart_remove(struct serdev_device *serdev)
>>> +{
>>> +	struct qcauart *qca = serdev_device_get_drvdata(serdev);
>>> +
>>> +	netif_carrier_off(qca->net_dev);
>>> +	cancel_work_sync(&qca->tx_work);
>>> +	unregister_netdev(qca->net_dev);
>>
>> Note that it is still possible that the tx work is queued right after cancel_work_sync()
>> returned and before the net device is unregistered (and thus the check for the net device
>> being up at the beginning of the tx work function is passed and the function is executed).
> 
> Even if the carrier is off? Since i see this pattern in some drivers, can you please point me to a reference like a thread or something else?
> 

The check in the tx work function is against the "running" state not against the carrier. So why should 
the carrier matter in this case? 


>> I suggest to avoid this possible race by first unregistering the netdevice and then 
>> calling cancel_work_sync().
> 
> What makes you sure that's safe to unregister the netdev while the tx work queue is possibly active?

unregister_netdevice() calls netdev_close() if the interface is still up. netdev_close() calls flush_work()
so the unregistration is delayed until the tx work function is finished. Furthermore both close() and
tx work are synchronized by means of the qca->lock which also guarantees that unregister_netdevice() wont
be finished until the tx work is done.

But I may have missed something and if unregistering the device while the tx work could be running worries you,
we could first close and later unregister the device like in the following sequence:


dev_close();
/* the tx work wont be scheduled any more now, however we have to wait for a potentially
   earlier scheduled work */ 
cancel_work_sync(&qca->tx_work);
/* we can be sure that the tx work will neither be running nor be started again, so 
   it is safe to unregister the netdev */
unregister_netdev(qca->net_dev);
serdev_device_close(serdev);
free_netdev(qca->net_dev);
 
What do you think?

Regards,
Lino