Message-ID: <0edbd57a-cf94-4710-a20c-04d0cc43cfad@gmail.com>
Date: Mon, 25 Dec 2023 22:57:23 +0800
From: Li Tuo <islituo@...il.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 s.nawrocki@...sung.com, mchehab@...nel.org, alim.akhtar@...sung.com
Cc: linux-media@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
 baijiaju1990@...il.com, stable@...r.kernel.org, BassCheck <bass@...a.edu.cn>
Subject: Re: [PATCH] media:fimc-capture: Fix a possible data inconsistency due
 to a data race in fimc_subdev_set_fmt()

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@...il.com>
>> Cc: stable@...r.kernel.org
>> Reported-by: BassCheck <bass@...a.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

Powered by blists - more mailing lists