dax: fix race condition on the tuning buffer - #11056
Conversation
It is necessary cause p_dax is used in dax.c now. Signed-off-by: Jun Lai <jun.lai@dolby.com>
|
Can one of the admins verify this patch?
|
| size); | ||
| if (adapter_data->tmp_tuning_buf.addr && adapter_data->tmp_tuning_buf.size > 0) { | ||
| dax_buffer_release(mod, &dax_ctx->tuning_file_buffer); | ||
| dax_ctx->tuning_file_buffer = adapter_data->tmp_tuning_buf; |
There was a problem hiding this comment.
not sure how this fixes the race... dax_buffer_release() in line 253 itself is racy - it frees directly the dax_buff->addr pointer and only then assigns NULL to it, so there's a use-after-free potential there. Also here line 254 isn't atomic - it's still the same old memcpy(), so, not very clear to me how this commit fixes any racing issues?
There was a problem hiding this comment.
Uha, my bad, I miss some changes. A spinlock is added to protect access to set_tuning_file.
set_tuning_file now works in process thread which has lower thread priority than set configuration, hence I dont add a spinlock in dax_set_param_wrapper.
for dax_buffer_release(), use-after-free will not happen because tuning buffer is only used in process thread through dax_find_params interface. They are always in the same thread.
13a3dbb to
0749fcc
Compare
While the tuning buffer is being updated, it may also be being used simultaneously in the process thread. Signed-off-by: Jun Lai <jun.lai@dolby.com>
0749fcc to
95f5201
Compare
| const char *dax_get_version(void) | ||
| { | ||
| return ""; | ||
| return "mock_version"; |
There was a problem hiding this comment.
does C guarantee that this string is allocated in some permanent section and not on stack?
There was a problem hiding this comment.
Yes, string literals are guaranteed to have static storage duration in C.
| set_tuning_file(mod, value, size); | ||
| if (dax_buffer_alloc(mod, &adapter_data->tmp_tuning_buf, size) != 0) { | ||
| comp_err(dev, "allocate %u bytes failed for tuning file", size); | ||
| ret = -ENOMEM; |
There was a problem hiding this comment.
I do see, that dax_buffer_alloc() currently only can return 0 or -ENOMEM but in general usually propagating errors is preferred.
| memcpy_s(adapter_data->tmp_tuning_buf.addr, | ||
| adapter_data->tmp_tuning_buf.free, | ||
| value, | ||
| size); |
| dax_ctx->tuning_file_buffer.free, | ||
| value, | ||
| size); | ||
| key = k_spin_lock(&adapter_data->lock); |
There was a problem hiding this comment.
I suppose you're implicitly using the fact, that this function only runs in IPC context, when its priority is lower than the processing function? Although IIRC you also support running in DP mode, not sure this priority promise will also always hold for that case. So I suppose you'd need to take the lock in the critical part of the processing function too - where this tuning buffer is used there.
There was a problem hiding this comment.
Good point, and I agree we should not rely on an IPC-priority assumption, especially with DP mode in mind.
Also, the active buffer (dax_ctx->tuning_file_buffer) is only switched from the processing path (check_and_update_settings() -> set_tuning_file()), so we do not need to lock the other parts of the processing function.
I think I should add a lock in dax_set_param_wrapper as well to prevent access to same tuning buffer object.
4 participants
No description provided.