Re: [alsa-devel] [v3 04/11] ASoC: Intel: sst: Add IPC handling
On Mon, Sep 01, 2014 at 07:27:07PM +0530, Subhransu S. Prusty wrote:
On Mon, Sep 01, 2014 at 01:51:14PM +0100, Mark Brown wrote:
On Mon, Sep 01, 2014 at 05:47:53PM +0530, Subhransu S. Prusty wrote:
I'd expect much louder complaints if we try to free something that's not allocated - what happens if we end up reallocating something quickly and then double freeing? Better to complain if we hit such a code path.
"freed" is a block which is passed by the caller to be freed up. Will add a comment.
How would that address the problem? Obviously the caller is trying to free what they're passing in.
sst_create_block() which allocates the memory and sst_free_block() which frees the memory are called in a synchronous way. A single thread who is allocating waits till a response arrives, if that response is valid then after processing the response the sst_free_block() is called to free up the memory. So the double freeing will not happen. Does this address your concern?
No. You've described what happens when things are working and everything is operating correctly and there are no bugs in the kernel, the goal with error checking is to provide robustness against the possibility that one of those things isn't true so we can tell what went wrong more easily than if we get memory corruption.
On Mon, Sep 01, 2014 at 03:41:34PM +0100, Mark Brown wrote:
On Mon, Sep 01, 2014 at 07:27:07PM +0530, Subhransu S. Prusty wrote:
On Mon, Sep 01, 2014 at 01:51:14PM +0100, Mark Brown wrote:
On Mon, Sep 01, 2014 at 05:47:53PM +0530, Subhransu S. Prusty wrote:
I'd expect much louder complaints if we try to free something that's not allocated - what happens if we end up reallocating something quickly and then double freeing? Better to complain if we hit such a code path.
"freed" is a block which is passed by the caller to be freed up. Will add a comment.
How would that address the problem? Obviously the caller is trying to free what they're passing in.
sst_create_block() which allocates the memory and sst_free_block() which frees the memory are called in a synchronous way. A single thread who is allocating waits till a response arrives, if that response is valid then after processing the response the sst_free_block() is called to free up the memory. So the double freeing will not happen. Does this address your concern?
No. You've described what happens when things are working and everything is operating correctly and there are no bugs in the kernel, the goal with error checking is to provide robustness against the possibility that one of those things isn't true so we can tell what went wrong more easily than if we get memory corruption.
Lets assume a wrong case here is triggered due to some other issue. So we get invoked twice for the same pointer. Since the function holds the lock and searches the object in the list, only first access will find the object and start to free it and relinquish the lock.
Now, the second access will not find this and return, so no harm done.
I agree that we need to at least put a log indicating such a scenario did occur and we failed to find the object. So we can return immediately after freeing up and then if we hit end of function implying we haven't found the object we should complain.
Would that help?
On Tue, Sep 02, 2014 at 10:52:25AM +0530, Vinod Koul wrote:
On Mon, Sep 01, 2014 at 03:41:34PM +0100, Mark Brown wrote:
No. You've described what happens when things are working and everything is operating correctly and there are no bugs in the kernel, the goal with error checking is to provide robustness against the possibility that one of those things isn't true so we can tell what went wrong more easily than if we get memory corruption.
Lets assume a wrong case here is triggered due to some other issue. So we get invoked twice for the same pointer. Since the function holds the lock and searches the object in the list, only first access will find the object and start to free it and relinquish the lock.
Now, the second access will not find this and return, so no harm done.
Consider the case where we do another allocation and happen to get a previously allocated address back; if we end up doing a double free then that would result in the new allocation being freed which would in turn lead to memory corruption problems. It's the sort of thing that's really unlikely to happen but can be a nightmare to debug when it does, a little bit of defensiveness early on can help a lot with avoiding having to deal with such issues.
I agree that we need to at least put a log indicating such a scenario did occur and we failed to find the object. So we can return immediately after freeing up and then if we hit end of function implying we haven't found the object we should complain.
Would that help?
That's exactly what I'm asking for, thanks.
participants (2)
-
Mark Brown -
Vinod Koul