Messages in this thread | | | From | Michael Kelley <> | Subject | RE: [PATCH v2 2/2] Drivers: hv: vmbus: Add a channel ring buffer mutex lock | Date | Sun, 24 Feb 2019 16:53:03 +0000 |
| |
From: Kimberly Brown <kimbrownkd@gmail.com> Sent: Thursday, February 21, 2019 7:47 PM > > The "_show" functions that access channel ring buffer data are > vulnerable to a race condition that can result in a NULL pointer > dereference. This problem was discussed here: > https://lkml.org/lkml/2018/10/18/779 > > To prevent this from occurring, add a new mutex lock, > "ring_buffer_mutex", to the vmbus_channel struct. > > Acquire/release "ring_buffer_mutex" in the functions that can set the > ring buffer pointer to NULL: vmbus_free_ring() and __vmbus_open(). > > Acquire/release "ring_buffer_mutex" in the four channel-level "_show" > functions that access ring buffer data. Remove the "const" qualifier > from the "struct vmbus_channel *chan" parameter of the channel-level > "_show" functions so that "ring_buffer_mutex" can be acquired/released > in these functions. > > Acquire/release "ring_buffer_mutex" in hv_ringbuffer_get_debuginfo(). > Pass the channel pointer to hv_ringbuffer_get_debuginfo() so that > "ring_buffer_mutex" can be accessed in this function. > > Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
I've reviewed the code. I believe it is correct and fixes the race condition. Unfortunately, the code ended up being messier than I had hoped, and in particular, the need to pass the channel pointer into the ring buffer functions is distasteful. An alternate idea is to put the new mutex into the hv_ring_buffer_info structure. This results in two mutex's since there's a separate hv_ring_buffer_info structure for the "in" ring and the "out" ring. But it makes the ring buffer functions more self-contained and able to operate without knowledge of the channel. The mutex can be obtained in hv_ringbuffer_cleanup() instead of in the vmbus functions, and hv_ringbuffer_get_debuginfo() doesn't need the channel pointer.
The "const" still has to dropped from the channel pointer because the hv_ring_buffer_info structures are inline in the channel structure, but that's less objectionable. The extra memory for two mutex's isn't really a problem, and none of the code paths are performance sensitive.
It's a tradeoff. I think I slightly prefer moving the mutex to the hv_ring_buffer_info structure, but could also be persuaded to take it like it is.
Thoughts?
Michael
| |