[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b651ae17-0d60-46b5-9571-cf82769ab07c@kernel.org>
Date: Mon, 19 Aug 2024 19:59:24 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Michael Ellerman <mpe@...erman.id.au>, cassel@...nel.org
Cc: linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, hch@....de, linux-ppc@...la.no,
vidra@...l.mff.cuni.cz
Subject: Re: [PATCH] ata: pata_macio: Fix DMA table overflow
On 8/19/24 19:17, Michael Ellerman wrote:
> Kolbjørn and Jonáš reported that their 32-bit PowerMacs were crashing
> in pata-macio since commit 09fe2bfa6b83 ("ata: pata_macio: Fix
> max_segment_size with PAGE_SIZE == 64K").
>
> For example:
>
> kernel BUG at drivers/ata/pata_macio.c:544!
> Oops: Exception in kernel mode, sig: 5 [#1]
> BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 DEBUG_PAGEALLOC PowerMac
> ...
> NIP pata_macio_qc_prep+0xf4/0x190
> LR pata_macio_qc_prep+0xfc/0x190
> Call Trace:
> 0xc1421660 (unreliable)
> ata_qc_issue+0x14c/0x2d4
> __ata_scsi_queuecmd+0x200/0x53c
> ata_scsi_queuecmd+0x50/0xe0
> scsi_queue_rq+0x788/0xb1c
> __blk_mq_issue_directly+0x58/0xf4
> blk_mq_plug_issue_direct+0x8c/0x1b4
> blk_mq_flush_plug_list.part.0+0x584/0x5e0
> __blk_flush_plug+0xf8/0x194
> __submit_bio+0x1b8/0x2e0
> submit_bio_noacct_nocheck+0x230/0x304
> btrfs_work_helper+0x200/0x338
> process_one_work+0x1a8/0x338
> worker_thread+0x364/0x4c0
> kthread+0x100/0x104
> start_kernel_thread+0x10/0x14
>
> That commit increased max_segment_size to 64KB, with the justification
> that the SCSI core was already using that size when PAGE_SIZE == 64KB,
> and that there was existing logic to split over-sized requests.
>
> However with a sufficiently large request, the splitting logic causes
> each sg to be split into two commands in the DMA table, leading to
> overflow of the DMA table, triggering the BUG_ON().
>
> With default settings the bug doesn't trigger, because the request size
> is limited by max_sectors_kb == 1280, however max_sectors_kb can be
> increased, and apparently some distros do that by default using udev
> rules.
>
> Fix the bug for 4KB kernels by reverting to the old max_segment_size.
>
> For 64KB kernels the sg_tablesize needs to be halved, to allow for the
> possibility that each sg will be split into two.
>
> Fixes: 09fe2bfa6b83 ("ata: pata_macio: Fix max_segment_size with PAGE_SIZE == 64K")
> Cc: stable@...r.kernel.org # v6.10+
> Reported-by: Kolbjørn Barmen <linux-ppc@...la.no>
> Closes: https://lore.kernel.org/all/62d248bb-e97a-25d2-bcf2-9160c518cae5@kolla.no/
> Reported-by: Jonáš Vidra <vidra@...l.mff.cuni.cz>
> Closes: https://lore.kernel.org/all/3b6441b8-06e6-45da-9e55-f92f2c86933e@ufal.mff.cuni.cz/
> Tested-by: Kolbjørn Barmen <linux-ppc@...la.no>
> Signed-off-by: Michael Ellerman <mpe@...erman.id.au>
> ---
> drivers/ata/pata_macio.c | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c
> index 1b85e8bf4ef9..eaffa510de49 100644
> --- a/drivers/ata/pata_macio.c
> +++ b/drivers/ata/pata_macio.c
> @@ -208,6 +208,19 @@ static const char* macio_ata_names[] = {
> /* Don't let a DMA segment go all the way to 64K */
> #define MAX_DBDMA_SEG 0xff00
>
> +#ifdef CONFIG_PAGE_SIZE_64KB
> +/*
> + * The SCSI core requires the segment size to cover at least a page, so
> + * for 64K page size kernels it must be at least 64K. However the
> + * hardware can't handle 64K, so pata_macio_qc_prep() will split large
> + * requests. To handle the split requests the tablesize must be halved.
> + */
> +#define MAX_SEGMENT_SIZE SZ_64K
> +#define SG_TABLESIZE (MAX_DCMDS / 2)
> +#else
> +#define MAX_SEGMENT_SIZE MAX_DBDMA_SEG
> +#define SG_TABLESIZE MAX_DCMDS
> +#endif
These names are rather generic and could clash with some core layer ditions. So
maybe prefix the macro names with PATA_MACIO_ ?
Also please tab-align the values to make this a little easier to read.
Other than this, looks good to me.
>
> /*
> * Wait 1s for disk to answer on IDE bus after a hard reset
> @@ -912,16 +925,10 @@ static int pata_macio_do_resume(struct pata_macio_priv *priv)
>
> static const struct scsi_host_template pata_macio_sht = {
> __ATA_BASE_SHT(DRV_NAME),
> - .sg_tablesize = MAX_DCMDS,
> + .sg_tablesize = SG_TABLESIZE,
> /* We may not need that strict one */
> .dma_boundary = ATA_DMA_BOUNDARY,
> - /*
> - * The SCSI core requires the segment size to cover at least a page, so
> - * for 64K page size kernels this must be at least 64K. However the
> - * hardware can't handle 64K, so pata_macio_qc_prep() will split large
> - * requests.
> - */
> - .max_segment_size = SZ_64K,
> + .max_segment_size = MAX_SEGMENT_SIZE,
> .device_configure = pata_macio_device_configure,
> .sdev_groups = ata_common_sdev_groups,
> .can_queue = ATA_DEF_QUEUE,
--
Damien Le Moal
Western Digital Research
Powered by blists - more mailing lists