Re: [PATCH] Bug fix for the s390 dcssblk driver

emist
2007-10-23T22:04:06Z

Gerald Schaefer wrote:
> On Mon, 2007-10-22 at 13:37 +0200, Cornelia Huck wrote:
>> On Sun, 21 Oct 2007 23:46:49 -0400,
>> emist <emistz@gmail.com> wrote:
>>
>>> # This patch fixes a memory corruption bug in the s390 dcssblk driver.
>>> # The bug occurs when an attempt to change the type of a segment
>>> # returns an error. At this point the driver tries to remove the segment in
>>> # question while some of the device's attributes are in use. This causes the
>>> # driver to hang.
>> Hm, seems we missed another of those device attributes exhibiting
>> suicidal tendencies...
>>
>> Tejun has a patchset allowing device attributes to commit suicide (see
>> http://marc.info/?l=linux-kernel&m=119027371416452&w=2), although I'm
>> not sure what its current status is. Until then, you would need to use
>> device_schedule_callback() to commit suicide.
>>
>> This all of course only applies if killing the segment is better than
>> leaving it in its current state, but others can make a better judgement
>> on that :)
>
> Hi,
>
> thanks for reporting this bug, seems like we forgot to consider the
> suicidal behavior of this driver when the device_unregister() stuff was
> changed.
>
> The best solution for now would be to use the scheduled callback
> function, like Cornelia described. If segment_modify_shared() should
> fail, the DCSS segment will be unloaded. Calling the function again
> with the old "shared" flag will not help because it will not reload
> the segment. So we need to remove/unregister the device in this error
> path, and for now this should be done with device_schedule_callback().
>
> Signed-off-by: Gerald Schaefer <gerald.schaefer@de.ibm.com>
> ---
>
> dcssblk.c | 9 +++++++--
> 1 files changed, 7 insertions(+), 2 deletions(-)
>
> Index: linux-2.6.23/drivers/s390/block/dcssblk.c
> ===================================================================
> --- linux-2.6.23.orig/drivers/s390/block/dcssblk.c
> +++ linux-2.6.23/drivers/s390/block/dcssblk.c
> @@ -193,6 +193,12 @@ dcssblk_segment_warn(int rc, char* seg_n
> }
> }
>
> +static void dcssblk_unregister_callback(struct device *dev)
> +{
> + device_unregister(dev);
> + put_device(dev);
> +}
> +
> /*
> * device attribute for switching shared/nonshared (exclusive)
> * operation (show + store)
> @@ -276,8 +282,7 @@ removeseg:
> blk_cleanup_queue(dev_info->dcssblk_queue);
> dev_info->gd->queue = NULL;
> put_disk(dev_info->gd);
> - device_unregister(dev);
> - put_device(dev);
> + rc = device_schedule_callback(dev, dcssblk_unregister_callback);
> out:
> up_write(&dcssblk_devices_sem);
> return rc;
>
>
Hey,

That makes sense. I no longer have access to an s390 system so I will
not be able to provide a tested patch for this bug. I will however
attempt to fix this issue and submit a patch using the scheduled
callback function and maybe someone could make sure that it works properly.

Have a good one,

Igor H.