| Message ID | 20250129225636.2667932-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | md: Fix linear_set_limits() | expand |
On Wed, Jan 29, 2025 at 02:56:35PM -0800, Bart Van Assche wrote: > queue_limits_cancel_update() must only be called if > queue_limits_start_update() is called first. Remove the > queue_limits_cancel_update() call from linear_set_limits() because > there is no corresponding queue_limits_start_update() call. Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> > This bug was discovered by annotating all mutex operations with clang > thread-safety attributes and by building the kernel with clang and > -Wthread-safety. Can you send patches for that?
On 1/30/25 11:49 PM, Christoph Hellwig wrote: > On Wed, Jan 29, 2025 at 02:56:35PM -0800, Bart Van Assche wrote: >> This bug was discovered by annotating all mutex operations with clang >> thread-safety attributes and by building the kernel with clang and >> -Wthread-safety. > > Can you send patches for that? Sure, but it will take a few additional days before these will be ready to be posted. My current plan is as follows: - In a first phase, annotate struct mutex and the mutex_lock()/mutex_unlock() calls and their variants only. This is sufficient to detect locking bugs at compile time in error paths and also to support GUARDED_BY() if neither the guard() macro nor the scoped_guard() macro are used. - Next, modify the clang compiler such that the guard() macro becomes supported. The challenge with the guard() macro is that it creates an alias for synchronization object pointers, that the cleanup function is passed a pointer to the synchronization object alias and also that alias analysis is not supported by the clang thread-safety analysis. I have not yet decided how to implement this. - Evaluate whether it's worth it to annotate other synchronization objects than struct mutex. Bart.
On Wed, Jan 29, 2025 at 2:57 PM Bart Van Assche <bvanassche@acm.org> wrote: > > queue_limits_cancel_update() must only be called if > queue_limits_start_update() is called first. Remove the > queue_limits_cancel_update() call from linear_set_limits() because > there is no corresponding queue_limits_start_update() call. > > This bug was discovered by annotating all mutex operations with clang > thread-safety attributes and by building the kernel with clang and > -Wthread-safety. > > Cc: Yu Kuai <yukuai3@huawei.com> > Cc: Coly Li <colyli@kernel.org> > Cc: Mike Snitzer <snitzer@kernel.org> > Cc: Christoph Hellwig <hch@lst.de> > Fixes: 127186cfb184 ("md: reintroduce md-linear") > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Applied to md-6.14 branch. Thanks for the fix! Song
diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c index a382929ce7ba..369aed044b40 100644 --- a/drivers/md/md-linear.c +++ b/drivers/md/md-linear.c @@ -76,10 +76,8 @@ static int linear_set_limits(struct mddev *mddev) lim.max_write_zeroes_sectors = mddev->chunk_sectors; lim.io_min = mddev->chunk_sectors << 9; err = mddev_stack_rdev_limits(mddev, &lim, MDDEV_STACK_INTEGRITY); - if (err) { - queue_limits_cancel_update(mddev->gendisk->queue); + if (err) return err; - } return queue_limits_set(mddev->gendisk->queue, &lim); }
queue_limits_cancel_update() must only be called if queue_limits_start_update() is called first. Remove the queue_limits_cancel_update() call from linear_set_limits() because there is no corresponding queue_limits_start_update() call. This bug was discovered by annotating all mutex operations with clang thread-safety attributes and by building the kernel with clang and -Wthread-safety. Cc: Yu Kuai <yukuai3@huawei.com> Cc: Coly Li <colyli@kernel.org> Cc: Mike Snitzer <snitzer@kernel.org> Cc: Christoph Hellwig <hch@lst.de> Fixes: 127186cfb184 ("md: reintroduce md-linear") Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/md/md-linear.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)