[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4575: net/devif: Fix the memory leak in case of netdev isn't alive

[GitBox <gi...@apache.org>]

xiaoxiang781216 commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-1006261687


   > > @xiaoxiang781216 I tried to find if changing dev->d_flags is synchronized to net_lock() anywhere. It looks like dev->d_flags can be modified (e.g. by psock_ioctl()) asynchronously to net_lock() / net_unlock critical sections. In this case dev->d_flags may change its state to IFF_DOWN right after you have checked (dev->d_flags & IFF_UP). Then the event callback will still be added to the event list while the network adapter is already down. Is it correct?
   > 
   > **Before this patch the algorithm was as follows:**
   > 
   > 1. net_lock() (actually this lock did not prevent dev->d_flags from changing outside)
   > 2. A callback slot was extracted from the g_cbfreelist list
   > 3. If the network interface appeared down (!(dev->d_flags & IFF_UP)), the callback slot returned to the g_cbfreelist list (actually it did not because devif_callback_free() was invoked with the second argument = NULL that was a bug), the operation was cancelled and exited with error.
   > 4. ...
   > 
   > Thus if the bug with NULL second argument of devif_callback_free() would be fixed (should be "ret" instead of "NULL"), the algorithm would guarantee that no excessive callback would be allocated.
   > 
   > **After this patch the algorithm is as follows:**
   > 
   > 1. net_lock() (this lock still does not prevent dev->d_flags from changing outside)
   > 2. If the network interface is down (!(dev->d_flags & IFF_UP)), exit with error.
   > 3. Else a callback slot is extracted from the g_cbfreelist list
   > 4. The callback added to the target list
   > 5. ...
   > 
   > In case of the new algorithm the following scenario is possible: Between steps 2 and 3 the network interface may go down (!(dev->d_flags & IFF_UP)) by netdev_ifdown() in netdev_ioctl.c. netdev_ifdown() is called e.g. via psock_ioctl(). There are not net_lock() / net_unlock(). Thus a new callback will be allocated by devif_callback_alloc() despite the network interface went down in between steps 2 and 3. At the same time, netdev_ifdown() (netdev_ioctl.c) does the following:
   > 
   > 1. ...
   > 2. dev->d_flags &= ~IFF_UP; (w/o any net_lock() / net_unlock())
   > 3. invokes devif_dev_event(dev, NULL, NETDEV_DOWN);
   > 4. ...
   > 
   
   The code before modification still suffer the issue you describe here: how to handle net devdown just after the check.
   
   > Actually the situation is not so bad because devif_dev_event() DOES contain net_lock() / net_unlock(). Thus devif_dev_event() will be suspended until devif_callback_alloc() completes. After that devif_dev_event() will notify all related network connections that the network interface has went down. And the network connection (that initiated previous devif_callback_alloc()) is responsible for deallocating the "hanging" excessively allocated callback. Thus the callback deallocation will be deffered.
   
   To fix this problem, it's better to add the lock in netdev_ifdown.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org