local: harden local_close() and use it for local_open() failure #575
Conversation
|
I don't know if @pcercuei or @larsclausen have time to have a peak at this one? |
What are the effects of the bug? Does it crash? |
Hmm, maybe I should describe it a bit better in the commit desc. So, issue is: Some resources seem to stay locked/leaked after I'll dig quick into the kernel and come back. |
So, after some quick debug-prints into the kernel, what seems to be happening is that this code does not run: When I add this patchset, the chrdev_release seems to be. I assumed that it may be a race-condition, and I added some sleep(1000)/wait [after the close()], it doesn't seem to be helping. The local_close() though, seems to be doing some |
A mapping on a file will keep a reference to that file. So this is probably the issue then. |
|
so, in local, the only mmap is in which should be released as part of when something calls |
|
Changelog v1 -> v2:
|
|
there are a few |
yeah; i also thought about that for a bit when doing the initial change; i was wondering if printing the error code from |
|
I think passing it up, and printing something might be worth it - then we can decide (based on user feedback) if it is a real problem or not. Won't be noisy, since I'm planning on starting the error message revamp in the next couple of weeks. |
alright; |
This also does a minor rework in re-defining the 'local_enable_buffer()' function into a 'local_buffer_enabled_set(..., bool en)' function which can enable/disable a local buffer by writing into the 'buffer/enable'. Keeping an internal state complicated things, as this state should be handled by the kernel. Even so, the original local_enable_buffer() would only get called once, while the 'local_open()' function would also bypass this state potentially causing inconsistency. Signed-off-by: Alexandru Ardelean <[email protected]>
There's a bug, when enabling a buffer (with local_buffer_enabled_set(), i.e. writing to 'buffer/enable'), we can get -EINVAL, because the scan_mask is invalidated by the kernel. The issue seems to be that the local_open() exit-on-failure path is un-balanced. Some things don't seem to be cleaned-up on exit-on-failure. This is seen with iiod trying to open a high-speed buffer device, where the exit-on-failure path does not call munmap() on the buffers, nor does it free the memory allocated for the buffer (in libiio). On the next retry/open the buffer device is locked and -EBUSY is returned. Essentially, this leaks resources (a file-descriptor and some memory for the buffers). Closing and re-opening iiod fixes the issue. Also, the local_close() function should not exit on the first fail in finds. It should continue with disabling everything it can. The only error that might make sense, is if the main FD is not initialized. This change simplifies the local_close() a bit: we return -EBADF if the main FD is -1. Other than that, we continue disabling/cleaning everything else (with the proper NULL/fd checks in place). This makes it re-usable for the local_open() exit-on-failure path. This could be addressed [also] by balancing the initialization in the reverse order of init. But it looks like something might be omitted later (as they were up until now) in the exit-on-failure path. So maybe a more future-proof method is to just re-use the local_open() routine where possible. Signed-off-by: Alexandru Ardelean <[email protected]>
With this change: * all error returns in local_close() get printed * the first error code is returned back to the caller Signed-off-by: Alexandru Ardelean <[email protected]>
|
I added a separate commit for the error handling in local_close(). I am not sure, the code has much added value, vs it's quantity. Also, since many places could return an error code in |
|
This looks good to me - thanks as long as @larsclausen doesn't have any comments... -Robin |
|
👍 |
|
this caused a warning in coverity... 1049 CID 361216 (#1 of 1): Logically dead code (DEADCODE) I think because, you are missing a |
yeah; it's a good catch on the side of coverity; |
The reason I asked for it - is there were error conditions on close() failing, that your previous patches removed. That is all. It's basically keeping those failing paths as they were - not adding more. |
There's a bug, when enabling a buffer (with local_buffer_enabled_set(),
i.e. writing to 'buffer/enable'), we can get -EINVAL, because the scan_mask
is invalidated by the kernel.
The issue seems to be that the local_open() exit-on-failure path is
un-balanced. Some things don't seem to be cleaned-up on exit-on-failure.
This is seen with iiod trying to open a high-speed buffer device, where the
exit-on-failure path does not call munmap() on the buffers, nor does it
free the memory allocated for the buffer (in libiio). On the next
retry/open the buffer device is locked and -EBUSY is returned.
Essentially, this leaks resources (a file-descriptor and some memory for
the buffers).
Closing and re-opening iiod fixes the issue.
Also, the local_close() function should not exit on the first fail in
finds. It should continue with disabling everything it can. The only error
that might make sense, is if the main FD is not initialized.
This change simplifies the local_close() a bit: we return -EBADF if the
main FD is -1. Other than that, we continue disabling/cleaning everything
else (with the proper NULL/fd checks in place).
This makes it re-usable for the local_open() exit-on-failure path.
This could be addressed [also] by balancing the initialization in the
reverse order of init. But it looks like something might be omitted later
(as they were up until now) in the exit-on-failure path.
So maybe a more future-proof method is to just re-use the local_open()
routine where possible.
Signed-off-by: Alexandru Ardelean [email protected]