lkml.org 
[lkml]   [2020]   [Nov]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 4.19 042/101] scsi: scsi_dh_alua: Avoid crash during alua_bus_detach()
    Hi!

    > From: Hannes Reinecke <hare@suse.de>
    >
    > [ Upstream commit 5faf50e9e9fdc2117c61ff7e20da49cd6a29e0ca ]
    >
    > alua_bus_detach() might be running concurrently with alua_rtpg_work(), so
    > we might trip over h->sdev == NULL and call BUG_ON(). The correct way of
    > handling it is to not set h->sdev to NULL in alua_bus_detach(), and call
    > rcu_synchronize() before the final delete to ensure that all concurrent
    > threads have left the critical section. Then we can get rid of the
    > BUG_ON() and replace it with a simple if condition.

    I don't get it.

    In the new version, h->sdev will never be NULL, because it is never
    set to NULL. It is will be valid up-to the point when h is freed...

    synchronize_rcu() should prevent race with alua_rtpg(), but BUG_ON()s
    should stay, to catch any bogosity... Or maybe the if (!h->sdev) tests
    should be simply removed. But it does not make sense to silently
    continue.

    Best regards,
    Pavel

    > +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
    > @@ -672,8 +672,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
    > rcu_read_lock();
    > list_for_each_entry_rcu(h,
    > &tmp_pg->dh_list, node) {
    > - /* h->sdev should always be valid */
    > - BUG_ON(!h->sdev);
    > + if (!h->sdev)
    > + continue;
    > h->sdev->access_state = desc[0];
    > }
    > rcu_read_unlock();
    > @@ -719,7 +719,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
    > pg->expiry = 0;
    > rcu_read_lock();
    > list_for_each_entry_rcu(h, &pg->dh_list, node) {
    > - BUG_ON(!h->sdev);
    > + if (!h->sdev)
    > + continue;
    > h->sdev->access_state =
    > (pg->state & SCSI_ACCESS_STATE_MASK);
    > if (pg->pref)
    > @@ -1160,7 +1161,6 @@ static void alua_bus_detach(struct scsi_device *sdev)
    > spin_lock(&h->pg_lock);
    > pg = rcu_dereference_protected(h->pg, lockdep_is_held(&h->pg_lock));
    > rcu_assign_pointer(h->pg, NULL);
    > - h->sdev = NULL;
    > spin_unlock(&h->pg_lock);
    > if (pg) {
    > spin_lock_irq(&pg->lock);
    > @@ -1169,6 +1169,7 @@ static void alua_bus_detach(struct scsi_device *sdev)
    > kref_put(&pg->kref, release_port_group);
    > }
    > sdev->handler_data = NULL;
    > + synchronize_rcu();
    > kfree(h);
    > }
    >

    --
    DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
    HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2020-11-18 23:18    [W:4.070 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site