| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5cfefec0-b64b-4f96-a943-4de3205d3c50@kernel.org>
Date: Fri, 8 Aug 2025 17:01:17 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Abinash Singh <abinashsinghlalotra@...il.com>,
James.Bottomley@...senPartnership.com
Cc: martin.petersen@...cle.com, linux-scsi@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] scsi: sd: Fix build warning in sd_revalidate_disk()
On 8/8/25 3:30 AM, Abinash Singh wrote:
> A build warning was triggered due to excessive stack usage in
> sd_revalidate_disk():
>
> drivers/scsi/sd.c: In function ‘sd_revalidate_disk.isra’:
> drivers/scsi/sd.c:3824:1: warning: the frame size of 1160 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>
> This is caused by a large local struct queue_limits (~400B) allocated
> on the stack. Replacing it with a heap allocation using kmalloc()
> significantly reduces frame usage. Kernel stack is limited (~8 KB),
> and allocating large structs on the stack is discouraged.
> As the function already performs heap allocations (e.g. for buffer),
> this change fits well.
>
> Signed-off-by: Abinash Singh <abinashsinghlalotra@...il.com>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4a68b2ab2804..a03844400e51 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -34,6 +34,7 @@
> */
>
> #include <linux/bio-integrity.h>
> +#include <linux/cleanup.h>
> #include <linux/module.h>
> #include <linux/fs.h>
> #include <linux/kernel.h>
> @@ -3696,11 +3696,16 @@ static int sd_revalidate_disk(struct gendisk *disk)
> struct scsi_disk *sdkp = scsi_disk(disk);
> struct scsi_device *sdp = sdkp->device;
> sector_t old_capacity = sdkp->capacity;
> - struct queue_limits lim;
> unsigned char *buffer;
> unsigned int dev_max;
> int err;
>
> + struct queue_limits *lim __free(kfree) = kmalloc(sizeof(*lim), GFP_KERNEL);
Please keep the declarations together. Not sure how that will work with that
unreadable __free(kfree) annotation though (the "unreadable" part of this
comment is only my opinion... I really dislike stuff that hides code...).
> + if (!lim) {
> + sd_printk(KERN_WARNING, sdkp, "sd_revalidate_disk: Memory allocation failure.\n");
Long line. Please split after sdkp. Also, the same message is used for the
buffer allocation. So maybe differentiate with it ? E.g. something like "Disk
limit allocation failure" ?
> + goto out;
> + }
> +
> SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
> "sd_revalidate_disk\n"));
>
> @@ -3720,14 +3726,14 @@ static int sd_revalidate_disk(struct gendisk *disk)
>
> sd_spinup_disk(sdkp);
>
> - lim = queue_limits_start_update(sdkp->disk->queue);
> + *lim = queue_limits_start_update(sdkp->disk->queue);
>
> /*
> * Without media there is no reason to ask; moreover, some devices
> * react badly if we do.
> */
> if (sdkp->media_present) {
> - sd_read_capacity(sdkp, &lim, buffer);
> + sd_read_capacity(sdkp, lim, buffer);
> /*
> * Some USB/UAS devices return generic values for mode pages
> * until the media has been accessed. Trigger a READ operation
> @@ -3741,17 +3747,17 @@ static int sd_revalidate_disk(struct gendisk *disk)
> * cause this to be updated correctly and any device which
> * doesn't support it should be treated as rotational.
> */
> - lim.features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
> + lim->features |= (BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM);
>
> if (scsi_device_supports_vpd(sdp)) {
> sd_read_block_provisioning(sdkp);
> - sd_read_block_limits(sdkp, &lim);
> + sd_read_block_limits(sdkp, lim);
> sd_read_block_limits_ext(sdkp);
> - sd_read_block_characteristics(sdkp, &lim);
> - sd_zbc_read_zones(sdkp, &lim, buffer);
> + sd_read_block_characteristics(sdkp, lim);
> + sd_zbc_read_zones(sdkp, lim, buffer);
> }
>
> - sd_config_discard(sdkp, &lim, sd_discard_mode(sdkp));
> + sd_config_discard(sdkp, lim, sd_discard_mode(sdkp));
>
> sd_print_capacity(sdkp, old_capacity);
>
> @@ -3761,45 +3767,45 @@ static int sd_revalidate_disk(struct gendisk *disk)
> sd_read_app_tag_own(sdkp, buffer);
> sd_read_write_same(sdkp, buffer);
> sd_read_security(sdkp, buffer);
> - sd_config_protection(sdkp, &lim);
> + sd_config_protection(sdkp, lim);
> }
>
> /*
> * We now have all cache related info, determine how we deal
> * with flush requests.
> */
> - sd_set_flush_flag(sdkp, &lim);
> + sd_set_flush_flag(sdkp, lim);
>
> /* Initial block count limit based on CDB TRANSFER LENGTH field size. */
> dev_max = sdp->use_16_for_rw ? SD_MAX_XFER_BLOCKS : SD_DEF_XFER_BLOCKS;
>
> /* Some devices report a maximum block count for READ/WRITE requests. */
> dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks);
> - lim.max_dev_sectors = logical_to_sectors(sdp, dev_max);
> + lim->max_dev_sectors = logical_to_sectors(sdp, dev_max);
>
> if (sd_validate_min_xfer_size(sdkp))
> - lim.io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
> + lim->io_min = logical_to_bytes(sdp, sdkp->min_xfer_blocks);
> else
> - lim.io_min = 0;
> + lim->io_min = 0;
>
> /*
> * Limit default to SCSI host optimal sector limit if set. There may be
> * an impact on performance for when the size of a request exceeds this
> * host limit.
> */
> - lim.io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
> + lim->io_opt = sdp->host->opt_sectors << SECTOR_SHIFT;
> if (sd_validate_opt_xfer_size(sdkp, dev_max)) {
> - lim.io_opt = min_not_zero(lim.io_opt,
> + lim->io_opt = min_not_zero(lim->io_opt,
> logical_to_bytes(sdp, sdkp->opt_xfer_blocks));
> }
>
> sdkp->first_scan = 0;
>
> set_capacity_and_notify(disk, logical_to_sectors(sdp, sdkp->capacity));
> - sd_config_write_same(sdkp, &lim);
> + sd_config_write_same(sdkp, lim);
> kfree(buffer);
>
> - err = queue_limits_commit_update_frozen(sdkp->disk->queue, &lim);
> + err = queue_limits_commit_update_frozen(sdkp->disk->queue, lim);
> if (err)
> return err;
>
--
Damien Le Moal
Western Digital Research
Powered by blists - more mailing lists