From: Vitaly Kuznetsov <vkuzn...@redhat.com> Date: Fri, 21 Oct 2016 17:17:18 +0200
> David Miller <da...@davemloft.net> writes: > >> From: Vitaly Kuznetsov <vkuzn...@redhat.com> >> Date: Fri, 21 Oct 2016 13:15:53 +0200 >> >>> David Miller <da...@davemloft.net> writes: >>> >>>> From: Vitaly Kuznetsov <vkuzn...@redhat.com> >>>> Date: Thu, 20 Oct 2016 10:51:04 +0200 >>>> >>>>> Stephen Hemminger <sthem...@microsoft.com> writes: >>>>> >>>>>> Do we need ACCESS_ONCE() here to avoid check/use issues? >>>>>> >>>>> >>>>> I think we don't: this is the only place in the function where we read >>>>> the variable so we'll get normal read. We're not trying to syncronize >>>>> with netvsc_init_buf() as that would require locking, if we read stale >>>>> NULL value after it was already updated on a different CPU we're fine, >>>>> we'll just return -EAGAIN. >>>> >>>> The concern is if we race with netvsc_destroy_buf() and this pointer >>>> becomes NULL after the test you are adding. >>> >>> Thanks, this is interesting. >>> >>> We may reach to netvsc_destroy_buf() by 3 pathes: >>> >>> 1) cleanup path in netvsc_init_buf(). It is never called with >>> send_section_map being not NULL so it seems we're safe. >>> >>> 2) from netvsc_remove() when the device is being removed. As far as I >>> understand we can't be on the transmit path after we call >>> unregister_netdev() so we're safe. >>> >>> 3) from netvsc_change_mtu() and netvsc_set_channels(). These pathes are >>> specific to netvsc driver as basically we need to remove the device and >>> add it back to change mtu/number of channels. The underligning 'struct >>> net_device' stays but the rest is being removed and added back. On both >>> pathes we first call netvsc_close() which does netif_tx_disable() and as >>> far as I understand (I may be wrong) this guarantees that we won't be in >>> netvsc_send(). >>> >>> So *I think* that we can't ever free send_section_map while in >>> netvsc_send() and we can't even get to netvsc_send() after it is freed >>> but I may have missed something. >> >> Ok you may be right. >> >> Can't the device be taken down by asynchronous events as well? For example >> if the peer end of the interface in the other guest disappears. > > The device may be hot removed from host's side. In this case we follow > the following call chain: > > ... -> vmbus_device_unregister() -> ... -> vmbus_remove() -> netvsc_remove() > > and netvsc_remove() does netif_tx_disable(); unregister_netdev(); > before calling rndis_filter_device_remove() leading to netvsc_destroy_buf(). > > So it seems we can't be in netvsc_send() when the device is > disappearing. Ok, it all looks good then.
