Fix data race in scsi cd driver
ClosedPublic
Actions

Authored by smalukav_gmail.com on Aug 30 2021, 11:36 AM.

Details

Reviewers

avg
mav

Group Reviewers

cam

Commits

rG964d2210e122: Fix data race in scsi cd driver.
rGe76786909ce0: Fix data race in scsi cd driver.

Summary

There is a data race between cdsysctlinit https://reviews.freebsd.org/source/src/browse/main/sys/cam/scsi/scsi_cd.c$508 and cdcheckmedia https://reviews.freebsd.org/source/src/browse/main/sys/cam/scsi/scsi_cd.c$2680 . Both functions change softc->flags without synchronization.

We encountered the following scenario:

One thread enters cdsysctlinit
Another thread enters g_raid_taste → g_access → cdopen → cdcheckmedia
Both threads change softc->flags at the same time
cdcheckmedia fails to set CD_FLAG_MEDIA_WAIT and hangs off there https://reviews.freebsd.org/source/src/browse/main/sys/cam/scsi/scsi_cd.c$2701

To fix we lock periph in cdsysctlinit

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

smalukav_gmail.com created this revision.Aug 30 2021, 11:36 AM

Herald added a reviewer: cam. · View Herald TranscriptAug 30 2021, 11:36 AM

smalukav_gmail.com requested review of this revision.Aug 30 2021, 11:36 AM

smalukav_gmail.com added reviewers: avg, mav.Aug 30 2021, 11:42 AM

lytboris_gmail.com added a subscriber: lytboris_gmail.com.Aug 30 2021, 11:55 AM

While it looks OK to me, I think it would be good for different drivers to be as close as possible. And I see da got their locking slightly different in af1823cde891.

In D31726#715924, @mav wrote:

While it looks OK to me, I think it would be good for different drivers to be as close as possible. And I see da got their locking slightly different in af1823cde891.

Do you mean using cam_periph_acquire and cam_periph_release_locked? I suppose we should ref the periph in cdsysctlinit. We use the same pattern in cdopen https://reviews.freebsd.org/source/src/browse/main/sys/cam/scsi/scsi_cd.c$756 .

In D31726#716029, @smalukav_gmail.com wrote:

In D31726#715924, @mav wrote:

While it looks OK to me, I think it would be good for different drivers to be as close as possible. And I see da got their locking slightly different in af1823cde891.

Do you mean using cam_periph_acquire and cam_periph_release_locked? I suppose we should ref the periph in cdsysctlinit. We use the same pattern in cdopen https://reviews.freebsd.org/source/src/browse/main/sys/cam/scsi/scsi_cd.c$756 .

I meant taking cam_periph_lock() just around softc->flags |= DA_FLAG_SCTX_INIT; to make drivers textually close. You can not add sysctls while holding periph lock, since the functions use M_WAITOK, that is why taskqueue is used at all. Plus while there, it would be good to review what else may need to be protected, alike to done in the mentioned patch.

ae added a subscriber: ae.Aug 31 2021, 3:13 AM

Move using periph lock to around softc->flags change. Add owning asserts.

Looks good to me.

This revision is now accepted and ready to land.Sep 11 2021, 4:21 PM

Closed by commit rGe76786909ce0: Fix data race in scsi cd driver. (authored by mav). · Explain WhySep 13 2021, 1:02 PM

This revision was automatically updated to reflect the committed changes.

mav added a commit: rGe76786909ce0: Fix data race in scsi cd driver..

mav added a commit: rG964d2210e122: Fix data race in scsi cd driver..Sep 27 2021, 12:54 AM