From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Damien Le Moal To: Hannes Reinecke , "linux-block@vger.kernel.org" , Jens Axboe , "linux-scsi@vger.kernel.org" , "Martin K . Petersen" , "dm-devel@redhat.com" , Mike Snitzer CC: Christoph Hellwig , Matias Bjorling Subject: Re: [PATCH v4 03/11] scsi: sd_zbc: Fix sd_zbc_check_zones() error checks Date: Fri, 12 Oct 2018 11:41:22 +0000 Message-ID: References: <20181012100850.23316-1-damien.lemoal@wdc.com> <20181012100850.23316-4-damien.lemoal@wdc.com> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 List-ID: On 2018/10/12 19:23, Hannes Reinecke wrote:=0A= > On 10/12/18 12:08 PM, Damien Le Moal wrote:=0A= >> The unsigned 32 bits overflow check for the zone size value is already= =0A= >> done within sd_zbc_check_zones() with the test:=0A= >>=0A= >> } else if (logical_to_sectors(sdkp->device, zone_blocks) > UINT_MAX) {= =0A= >>=0A= >> so there is no need to check again for an out of range value in=0A= >> sd_zbc_read_zones(). Simplify the code and fix sd_zbc_check_zones()=0A= >> error return to -EFBIG instead of -ENODEV if the zone size is too large.= =0A= >> Change the return type of sd_zbc_check_zones() to an int for the error= =0A= >> code and return the zone size (zone_blocks) through a u32 pointer to=0A= >> avoid overflowing the signed 32 return value.=0A= >>=0A= >> Signed-off-by: Damien Le Moal =0A= >> ---=0A= >> drivers/scsi/sd_zbc.c | 19 ++++++++-----------=0A= >> 1 file changed, 8 insertions(+), 11 deletions(-)=0A= >>=0A= >> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c=0A= >> index ca73c46931c0..0678e1e108b0 100644=0A= >> --- a/drivers/scsi/sd_zbc.c=0A= >> +++ b/drivers/scsi/sd_zbc.c=0A= >> @@ -373,7 +373,7 @@ static int sd_zbc_check_zoned_characteristics(struct= scsi_disk *sdkp,=0A= >> * Returns the zone size in number of blocks upon success or an error = code=0A= >> * upon failure.=0A= >> */=0A= >> -static s64 sd_zbc_check_zones(struct scsi_disk *sdkp)=0A= >> +static int sd_zbc_check_zones(struct scsi_disk *sdkp, u32 *zblocks)=0A= >> {=0A= >> u64 zone_blocks =3D 0;=0A= >> sector_t max_lba, block =3D 0;=0A= > =0A= > Actually I thought to just change the 's32' to 'int', and not adding =0A= > another parameter; but anyway.=0A= =0A= Yes, I understood that. But since chunk_sectors is unsigned int, zone_block= s has=0A= to be too and so returning that through an int would be asking for troubles= . If=0A= we ever have see a drive with a 2G LBA zone size that is :)=0A= I thought it was cleaner this way.=0A= =0A= > =0A= > Reviewed-by: Hannes Reinecke =0A= =0A= Thanks !=0A= =0A= > =0A= > Hannes=0A= > =0A= =0A= =0A= -- =0A= Damien Le Moal=0A= Western Digital Research=0A= From mboxrd@z Thu Jan 1 00:00:00 1970 From: Damien Le Moal Subject: Re: [PATCH v4 03/11] scsi: sd_zbc: Fix sd_zbc_check_zones() error checks Date: Fri, 12 Oct 2018 11:41:22 +0000 Message-ID: References: <20181012100850.23316-1-damien.lemoal@wdc.com> <20181012100850.23316-4-damien.lemoal@wdc.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com To: Hannes Reinecke , "linux-block@vger.kernel.org" , Jens Axboe , "linux-scsi@vger.kernel.org" , "Martin K . Petersen" , "dm-devel@redhat.com" , Mike Snitzer Cc: Christoph Hellwig , Matias Bjorling List-Id: linux-scsi@vger.kernel.org On 2018/10/12 19:23, Hannes Reinecke wrote: > On 10/12/18 12:08 PM, Damien Le Moal wrote: >> The unsigned 32 bits overflow check for the zone size value is already >> done within sd_zbc_check_zones() with the test: >> >> } else if (logical_to_sectors(sdkp->device, zone_blocks) > UINT_MAX) { >> >> so there is no need to check again for an out of range value in >> sd_zbc_read_zones(). Simplify the code and fix sd_zbc_check_zones() >> error return to -EFBIG instead of -ENODEV if the zone size is too large. >> Change the return type of sd_zbc_check_zones() to an int for the error >> code and return the zone size (zone_blocks) through a u32 pointer to >> avoid overflowing the signed 32 return value. >> >> Signed-off-by: Damien Le Moal >> --- >> drivers/scsi/sd_zbc.c | 19 ++++++++----------- >> 1 file changed, 8 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c >> index ca73c46931c0..0678e1e108b0 100644 >> --- a/drivers/scsi/sd_zbc.c >> +++ b/drivers/scsi/sd_zbc.c >> @@ -373,7 +373,7 @@ static int sd_zbc_check_zoned_characteristics(struct scsi_disk *sdkp, >> * Returns the zone size in number of blocks upon success or an error code >> * upon failure. >> */ >> -static s64 sd_zbc_check_zones(struct scsi_disk *sdkp) >> +static int sd_zbc_check_zones(struct scsi_disk *sdkp, u32 *zblocks) >> { >> u64 zone_blocks = 0; >> sector_t max_lba, block = 0; > > Actually I thought to just change the 's32' to 'int', and not adding > another parameter; but anyway. Yes, I understood that. But since chunk_sectors is unsigned int, zone_blocks has to be too and so returning that through an int would be asking for troubles. If we ever have see a drive with a 2G LBA zone size that is :) I thought it was cleaner this way. > > Reviewed-by: Hannes Reinecke Thanks ! > > Hannes > -- Damien Le Moal Western Digital Research