Messages in this thread | | | Date | Mon, 25 Dec 2023 22:57:23 +0800 | Subject | Re: [PATCH] media:fimc-capture: Fix a possible data inconsistency due to a data race in fimc_subdev_set_fmt() | From | Li Tuo <> |
| |
Hi Krzysztof,
Thanks for your reply very much. It is very helpful. I am really sorry to confuse you.
On 2023/12/24 18:11, Krzysztof Kozlowski wrote:
> On 23/12/2023 17:43, Tuo Li wrote: >> Accesses to ctx->s_frame.width and ctx->s_frame.height should be protected >> by the lock fimc->lock to guarantee that width and height are consistent. >> Here is an example in fimc_subdev_get_fmt(): >> >> struct fimc_frame *ff = &ctx->s_frame; // Alias >> mutex_lock(&fimc->lock); >> mf->width = ff->width; >> mf->height = ff->height; >> >> However, ctx->s_frame.width and ctx->s_frame.height are accessed without >> holding the lock fimc->lock in fimc_subdev_set_fmt(): >> >> mf->width = ctx->s_frame.width; >> mf->height = ctx->s_frame.height; > Other places setting parts of s_frame, like fimc_capture_try_format() or > fimc_capture_try_selection(), do not have mutex. > >> And thus a harmful data race can occur, which can make ctx->s_frame.width > Harmful how?
The function set_frame_crop() which updates s_frame.width and s_frame.height is called by fimc_cap_s_selection(). fimc_cap_s_selection() is an ioctl function and it is likely to be able to execute concurrently with other functions such as fimc_subdev_set_fmt(). However, in fimc_subdev_set_fmt(), the accesses to s_frame.width and s_frame.height are not protected by the mutex lock fimc->lock.
If fimc_cap_s_selection() and fimc_subdev_set_fmt() can execute concurrently and the execution orders is like this:
1. ctx->s_frame.width is accessed and assigned to mf->width in fimc_subdev_set_fmt() Line 1552 in fimc-capture.c 2. ctx->s_frame.width and ctx->s_frame.height is updated by fimc_cap_s_selection() Line 1329 in fimc-capture.c 3. ctx->s_frame.height is accessed and assigned to mf->height in fimc_subdev_set_fmt() Line 1553 in fimc-capture.c
The width and height assigned to mf are not coming from the same ctx->s_frame configuration and can cause data inconsistency.
Besides, the functions fimc_subdev_set_selection() and fimc_subdev_set_fmt() exist in the same driver interface named fimc_subdev_pad_ops. Therefore, it seems that fimc_subdev_set_fmt() and fimc_subdev_set_selection() can also execute concurrently. However, if the execution order of fimc_subdev_set_selection() and fimc_subdev_set_fmt() is like mentioned above, a data inconsistency can also occur.
I analyze this possible data race manually according to the code logic, and I am not sure whether all accesses to configurations such as width, height, f_width, and f_height should be protected by a mutex lock to make them consistent. I am really sorry to trouble you, and any feedback will be appreciated!
>> inconsistent with ctx->s_frame.height, if ctx->s_frame.height is updated >> right after ctx->s_frame.width is accessed by another thread. >> >> This possible bug is found by an experimental static analysis tool >> developed by our team, BassCheck[1]. This tool analyzes the locking APIs >> to extract function pairs that can be concurrently executed, and then >> analyzes the instructions in the paired functions to identify possible >> concurrency bugs including data races and atomicity violations. The above >> possible bug is reported when our tool analyzes the source code of >> Linux 6.2. >> >> To fix this possible data race, the lock operation mutex_lock(&fimc->lock) >> is moved to the front of the accesses to these two variables. With this >> patch applied, our tool no longer reports the bug, with the kernel >> configuration allyesconfig for x86_64. Due to the lack of associated >> hardware, we cannot test the patch in runtime testing, and just verify it >> according to the code logic. > You wrote long four paragraphs which have basically almost zero relevant > information, whether this locking is needed or not. Your bass > description is not relevant... or actually making things worse because I > am certain you are fixing it just to fix your report, not to fix real issue. > >> [1] https://sites.google.com/view/basscheck/ > Instead provide the report.
I am sorry to confuse you, and I wrote these descriptions according to researcher guidelines in the kernel documentation Documentation/process/researcher-guidelines.rst I will provide a more concise patch in the future.
>> Fixes: 88fa8311ee36 ("[media] s5p-fimc: Add support for ISP Writeback ...") >> Signed-off-by: Tuo Li <islituo@gmail.com> >> Cc: stable@vger.kernel.org >> Reported-by: BassCheck <bass@buaa.edu.cn> > Run checkpatch, you will see the warning.
Thanks for your advice. I am sorry to bother you, and I will be careful in the subsequent patches.
> Best regards, > Krzysztof > Sincerely, Tuo Li
| |