Re: [PATCH v5 07/10] scsi: sd_zbc: emulate ZONE_APPEND commands

[Johannes Thumshirn <Johannes.Thumshirn@wdc.com>]

On 10/04/2020 08:39, Christoph Hellwig wrote:
> Looking more the situation seems even worse.  If scsi_mq_prep_fn
> isn't successfull we never seem to free the sgtables, even for fatal
> errors.  So I think we need a real bug fix here in front of the series

If I'm not missing something all that needs to be done to fix it is:

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 4724002627cd..5e6165246f77 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1191,6 +1191,7 @@ static blk_status_t scsi_setup_cmnd(struct 
scsi_device *sdev,
                 struct request *req)
  {
         struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
+       blk_status_t ret;

         if (!blk_rq_bytes(req))
                 cmd->sc_data_direction = DMA_NONE;
@@ -1200,9 +1201,14 @@ static blk_status_t scsi_setup_cmnd(struct 
scsi_device *sdev,
                 cmd->sc_data_direction = DMA_FROM_DEVICE;

         if (blk_rq_is_scsi(req))
-               return scsi_setup_scsi_cmnd(sdev, req);
+               ret = scsi_setup_scsi_cmnd(sdev, req);
         else
-               return scsi_setup_fs_cmnd(sdev, req);
+               ret = scsi_setup_fs_cmnd(sdev, req);
+
+       if (ret != BLK_STS_OK)
+               scsi_free_sgtables(cmd);
+
+       return ret;
  }

  static blk_status_t


Theoretically it's enough to catch errors from scsi_setup_fs_cmnd() as 
scsi_setup_scsi_cmnd() either fails scsi_init_io() which means no 
sgtables are allocated or returns BLK_STS_OK.

But for the sake of symmetry and defensive programming I think we can 
also check the return of scsi_setup_scsi_cmnd(). I've checked 
scsi_free_sgtables() and __sg_free_table() and they're double-free safe.

Thoughts?