You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/09/19 02:10:38 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #4575: net/devif: Fix the memory leak in the error path of devif_callback_alloc
xiaoxiang781216 opened a new pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575
## Summary
## Impact
Avoid the memory leak in the error patch
## Testing
--
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
[GitHub] [incubator-nuttx] a-lunev commented on pull request #4575: net/devif: Simplify the logic in the error path of devif_callback_alloc
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-926305154
> @a-lunev but my change make the fail path more simple.
@xiaoxiang781216 There are pros and cons of each way.
The existing code seems having better performance (no locks/unlocks of d_flags), however it's slightly complex in this local place in devif_callback.c file.
Otherwise, there is a need to synchronize access to d_flags (adding locks/unlocks in multiple places of several files) that may reduce performance.
--
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
[GitHub] [incubator-nuttx] a-lunev commented on pull request #4575: net/devif: Simplify the logic in the error path of devif_callback_alloc
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-924287019
> > @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?
>
> Look like we need hold the lock before modifying d_flags.
So far I think it's not needed.
I would change only this single line:
```
git diff
diff --git a/net/devif/devif_callback.c b/net/devif/devif_callback.c
index 69a8b249c1..aa33695c23 100644
--- a/net/devif/devif_callback.c
+++ b/net/devif/devif_callback.c
@@ -283,7 +283,7 @@ FAR struct devif_callback_s *
{
/* No.. release the callback structure and fail */
- devif_callback_free(NULL, NULL, list_head, list_tail);
+ devif_callback_free(NULL, ret, list_head, list_tail);
net_unlock();
return NULL;
}
```
--
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
[GitHub] [incubator-nuttx] a-lunev edited a comment on pull request #4575: net/devif: Simplify the logic in the error path of devif_callback_alloc
Posted by GitBox <gi...@apache.org>.
a-lunev edited a comment on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-924272301
> @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?
@xiaoxiang781216 I've analyzed the source code once again. I've noticed that for some reason devif_callback_free() is invoked with the second argument = NULL:
`devif_callback_free(NULL, NULL, list_head, list_tail);`
The second argument of devif_callback_free() is "FAR struct devif_callback_s *cb".
If the second argument = NULL, then devif_callback_free() does not do anything and just returns. That will result in a memory leak as the callback structure will not be released.
As I understand, the following correction is required:
```
git diff
diff --git a/net/devif/devif_callback.c b/net/devif/devif_callback.c
index 69a8b249c1..aa33695c23 100644
--- a/net/devif/devif_callback.c
+++ b/net/devif/devif_callback.c
@@ -283,7 +283,7 @@ FAR struct devif_callback_s *
{
/* No.. release the callback structure and fail */
- devif_callback_free(NULL, NULL, list_head, list_tail);
+ devif_callback_free(NULL, ret, list_head, list_tail);
net_unlock();
return NULL;
}
```
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4575: net/devif: Simplify the logic in the error path of devif_callback_alloc
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-923880880
> Seems OK to me on the surface.
>
> The function's docstring says assumptions: this function is called with the network locked. Is that a stale comment?
No, after search all the invocation place, the comment is correct, but the code is too conservative. Do you want to remove the lock/unlock? I can provide a patch for this.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4575: net/devif: Simplify the logic in the error path of devif_callback_alloc
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-923880880
> Seems OK to me on the surface.
>
> The function's docstring says assumptions: this function is called with the network locked. Is that a stale comment?
@hartmannathan after search all the invocation place, the comment is correct, but the code is too conservative. Do you want to remove the lock/unlock? I can provide a patch for this.
--
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
[GitHub] [incubator-nuttx] a-lunev commented on pull request #4575: net/devif: Simplify the logic in the error path of devif_callback_alloc
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-924272301
> @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?
@xiaoxiang781216 I've analyzed the source code once again. For some reason devif_callback_free() is invoked with the second argument = NULL:
`devif_callback_free(NULL, NULL, list_head, list_tail);`
The second argument of devif_callback_free() is "FAR struct devif_callback_s *cb".
If the second argument = NULL, then devif_callback_free() does not do anything and just returns. That will result in a memory leak as the callback structure will not be released.
As I understand, the following correction is required:
```
git diff
diff --git a/net/devif/devif_callback.c b/net/devif/devif_callback.c
index 69a8b249c1..aa33695c23 100644
--- a/net/devif/devif_callback.c
+++ b/net/devif/devif_callback.c
@@ -283,7 +283,7 @@ FAR struct devif_callback_s *
{
/* No.. release the callback structure and fail */
- devif_callback_free(NULL, NULL, list_head, list_tail);
+ devif_callback_free(NULL, ret, list_head, list_tail);
net_unlock();
return NULL;
}
```
--
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
[GitHub] [incubator-nuttx] a-lunev commented on pull request #4575: net/devif: Fix the memory leak in case of netdev isn't alive
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-1006602259
> To fix this problem, it's better to add the lock in netdev_ifdown.
In that case just turning the network interface down will be deferred. And since netdev_ifdown() is invoked (and locked) the allocated callback (by devif_callback_alloc()) will be deallocated via devif_dev_event() notification. Thus adding net_lock() / net_unlock() to netdev_ifdown() seems not useful.
I created PR #5181 to remove devif_callback_free() call from devif_callback_alloc(). Also I added a comment into the code based on our discussion.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4575: net/devif: Simplify the logic in the error path of devif_callback_alloc
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-924557189
@a-lunev but my change make the fail path more simple.
--
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
[GitHub] [incubator-nuttx] a-lunev edited a comment on pull request #4575: net/devif: Simplify the logic in the error path of devif_callback_alloc
Posted by GitBox <gi...@apache.org>.
a-lunev edited a comment on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-924272301
> @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?
@xiaoxiang781216 I've analyzed the source code once again. As I've noticed, for some reason devif_callback_free() is invoked with the second argument = NULL:
`devif_callback_free(NULL, NULL, list_head, list_tail);`
The second argument of devif_callback_free() is "FAR struct devif_callback_s *cb".
If the second argument = NULL, then devif_callback_free() does not do anything and just returns. That will result in a memory leak as the callback structure will not be released.
As I understand, the following correction is required:
```
git diff
diff --git a/net/devif/devif_callback.c b/net/devif/devif_callback.c
index 69a8b249c1..aa33695c23 100644
--- a/net/devif/devif_callback.c
+++ b/net/devif/devif_callback.c
@@ -283,7 +283,7 @@ FAR struct devif_callback_s *
{
/* No.. release the callback structure and fail */
- devif_callback_free(NULL, NULL, list_head, list_tail);
+ devif_callback_free(NULL, ret, list_head, list_tail);
net_unlock();
return NULL;
}
```
--
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
[GitHub] [incubator-nuttx] a-lunev commented on pull request #4575: net/devif: Fix the memory leak in case of netdev isn't alive
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-1005946086
> @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. ...
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.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4575: net/devif: Fix the memory leak in case of netdev isn't alive
Posted by 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
[GitHub] [incubator-nuttx] a-lunev commented on pull request #4575: net/devif: Simplify the logic in the error path of devif_callback_alloc
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-926765238
> No, the old code hold the lock before checking the d_flags.
As I wrote before, I was not able to find places where writing to d_flags would be synchronized anywhere. So locking / unlocking d_flags for reading does not matter.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4575: net/devif: Simplify the logic in the error path of devif_callback_alloc
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-922693228
> @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?
Look like we need hold the lock before modifying d_flags.
--
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
[GitHub] [incubator-nuttx] anchao commented on pull request #4575: net/devif: Fix the memory leak in case of netdev isn't alive
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-1005709861
LGTM
--
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
[GitHub] [incubator-nuttx] a-lunev commented on pull request #4575: net/devif: Simplify the logic in the error path of devif_callback_alloc
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-922597930
@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?
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4575: net/devif: Simplify the logic in the error path of devif_callback_alloc
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575#issuecomment-926378610
> > @a-lunev but my change make the fail path more simple.
>
> @xiaoxiang781216 There are pros and cons of each way.
> The existing code seems having better performance (no locks/unlocks of d_flags), however it's slightly complex in this local place in devif_callback.c file.
No, the old code hold the lock before checking the d_flags.
> Otherwise, there is a need to synchronize access to d_flags (adding locks/unlocks in multiple places of several files) that may reduce performance.
The change keep the same lock strategy as before, the real change is moving the error check before allocating the callback structure.
--
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
[GitHub] [incubator-nuttx] anchao merged pull request #4575: net/devif: Fix the memory leak in case of netdev isn't alive
Posted by GitBox <gi...@apache.org>.
anchao merged pull request #4575:
URL: https://github.com/apache/incubator-nuttx/pull/4575
--
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