Re: [PATCH] media:fimc-capture: Fix a possible data inconsistency due to a data race in fimc_subdev_set_fmt()

[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@xxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reported-by: BassCheck <bass@xxxxxxxxxxx>
> 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