| 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
| ||
|
Date: Mon, 11 Aug 2008 12:52:19 -0700
From: "Grant Grundler" <grundler@...gle.com>
To: "Stefan Richter" <stefanr@...6.in-berlin.de>
Cc: linux1394-devel@...ts.sourceforge.net,
"FUJITA Tomonori" <fujita.tomonori@....ntt.co.jp>,
linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firewire: fw-sbp2: enforce s/g segment size limit
On Sat, Aug 9, 2008 at 11:21 AM, Stefan Richter
<stefanr@...6.in-berlin.de> wrote:
> 1. We don't need to round the SBP-2 segment size limit down to a
> multiple of 4 kB (0xffff -> 0xf000). It is only necessary to
> ensure quadlet alignment (0xffff -> 0xfffc).
>
> 2. Use dma_set_max_seg_size() to tell the DMA mapping infrastructure
> and the block IO layer about the restriction. This way we can
> remove the size checks and segment splitting in the queuecommand
> path.
>
> This assumes that no other code in the firewire stack uses
> dma_map_sg() with conflicting requirements. It furthermore assumes
> that the controller device's platform actually allows us to set the
> segment size to our liking. Assert the latter with a BUG_ON().
>
> 3. Also use blk_queue_max_segment_size() to tell the block IO layer
> about it. It cannot know it because our scsi_add_host() does not
> point to the FireWire controller's device.
>
> Signed-off-by: Stefan Richter <stefanr@...6.in-berlin.de>
> ---
> drivers/firewire/fw-sbp2.c | 68 +++++++++++++++++--------------------
> 1 file changed, 33 insertions(+), 35 deletions(-)
>
> Index: linux-2.6.27-rc2/drivers/firewire/fw-sbp2.c
> ===================================================================
> --- linux-2.6.27-rc2.orig/drivers/firewire/fw-sbp2.c
> +++ linux-2.6.27-rc2/drivers/firewire/fw-sbp2.c
> @@ -29,6 +29,7 @@
> */
>
> #include <linux/blkdev.h>
> +#include <linux/bug.h>
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/dma-mapping.h>
> @@ -181,10 +182,16 @@ struct sbp2_target {
> #define SBP2_MAX_LOGIN_ORB_TIMEOUT 40000U /* Timeout in ms */
> #define SBP2_ORB_TIMEOUT 2000U /* Timeout in ms */
> #define SBP2_ORB_NULL 0x80000000
> -#define SBP2_MAX_SG_ELEMENT_LENGTH 0xf000
> #define SBP2_RETRY_LIMIT 0xf /* 15 retries */
> #define SBP2_CYCLE_LIMIT (0xc8 << 12) /* 200 125us cycles */
>
> +/*
> + * The default maximum s/g segment size of a FireWire controller is
> + * usually 0x10000, but SBP-2 only allows 0xffff. Since buffers have to
> + * be quadlet-aligned, we set the length limit to 0xffff & ~3.
> + */
> +#define SBP2_MAX_SEG_SIZE 0xfffc
> +
> /* Unit directory keys */
> #define SBP2_CSR_UNIT_CHARACTERISTICS 0x3a
> #define SBP2_CSR_FIRMWARE_REVISION 0x3c
> @@ -1099,6 +1106,10 @@ static int sbp2_probe(struct device *dev
> struct Scsi_Host *shost;
> u32 model, firmware_revision;
>
> + if (dma_get_max_seg_size(device->card->device) > SBP2_MAX_SEG_SIZE)
> + BUG_ON(dma_set_max_seg_size(device->card->device,
> + SBP2_MAX_SEG_SIZE));
> +
> shost = scsi_host_alloc(&scsi_driver_template, sizeof(*tgt));
> if (shost == NULL)
> return -ENOMEM;
> @@ -1347,14 +1358,12 @@ static int
> sbp2_map_scatterlist(struct sbp2_command_orb *orb, struct fw_device *device,
> struct sbp2_logical_unit *lu)
> {
> - struct scatterlist *sg;
> - int sg_len, l, i, j, count;
> - dma_addr_t sg_addr;
> -
> - sg = scsi_sglist(orb->cmd);
> - count = dma_map_sg(device->card->device, sg, scsi_sg_count(orb->cmd),
> - orb->cmd->sc_data_direction);
> - if (count == 0)
> + struct scatterlist *sg = scsi_sglist(orb->cmd);
> + int i, j, len;
> +
> + i = dma_map_sg(device->card->device, sg, scsi_sg_count(orb->cmd),
> + orb->cmd->sc_data_direction);
> + if (i == 0)
> goto fail;
>
> /*
> @@ -1364,7 +1373,7 @@ sbp2_map_scatterlist(struct sbp2_command
> * as the second generation iPod which doesn't support page
> * tables.
> */
> - if (count == 1 && sg_dma_len(sg) < SBP2_MAX_SG_ELEMENT_LENGTH) {
> + if (i == 1) {
> orb->request.data_descriptor.high =
> cpu_to_be32(lu->tgt->address_high);
> orb->request.data_descriptor.low =
> @@ -1374,29 +1383,15 @@ sbp2_map_scatterlist(struct sbp2_command
> return 0;
> }
>
> - /*
> - * Convert the scatterlist to an sbp2 page table. If any
> - * scatterlist entries are too big for sbp2, we split them as we
> - * go. Even if we ask the block I/O layer to not give us sg
> - * elements larger than 65535 bytes, some IOMMUs may merge sg elements
> - * during DMA mapping, and Linux currently doesn't prevent this.
> - */
> - for (i = 0, j = 0; i < count; i++, sg = sg_next(sg)) {
> - sg_len = sg_dma_len(sg);
> - sg_addr = sg_dma_address(sg);
> - while (sg_len) {
> - /* FIXME: This won't get us out of the pinch. */
> - if (unlikely(j >= ARRAY_SIZE(orb->page_table))) {
> - fw_error("page table overflow\n");
> - goto fail_page_table;
> - }
> - l = min(sg_len, SBP2_MAX_SG_ELEMENT_LENGTH);
> - orb->page_table[j].low = cpu_to_be32(sg_addr);
> - orb->page_table[j].high = cpu_to_be32(l << 16);
I didn't check the rest of the driver - but it would be good if it
explicitly called dma_set_mask() or pci_dma_set_mask() with a 32-bit
mask value. Most drivers assume 32-bit and that's why I point this
out.
The rest looks ok at first glance.
thanks,
grant
> - sg_addr += l;
> - sg_len -= l;
> - j++;
> - }
> + j = 0;
> + for_each_sg(sg, sg, scsi_sg_count(orb->cmd), i) {
> + len = sg_dma_len(sg);
> + if (len == 0)
> + continue;
> +
> + orb->page_table[j].high = cpu_to_be32(len << 16);
> + orb->page_table[j].low = cpu_to_be32(sg_dma_address(sg));
> + j++;
> }
>
> orb->page_table_bus =
> @@ -1420,8 +1415,9 @@ sbp2_map_scatterlist(struct sbp2_command
> return 0;
>
> fail_page_table:
> - dma_unmap_sg(device->card->device, sg, scsi_sg_count(orb->cmd),
> - orb->cmd->sc_data_direction);
> + orb->page_table_bus = 0;
> + dma_unmap_sg(device->card->device, scsi_sglist(orb->cmd),
> + scsi_sg_count(orb->cmd), orb->cmd->sc_data_direction);
> fail:
> return -ENOMEM;
> }
> @@ -1542,6 +1538,8 @@ static int sbp2_scsi_slave_configure(str
> if (lu->tgt->workarounds & SBP2_WORKAROUND_128K_MAX_TRANS)
> blk_queue_max_sectors(sdev->request_queue, 128 * 1024 / 512);
>
> + blk_queue_max_segment_size(sdev->request_queue, SBP2_MAX_SEG_SIZE);
> +
> return 0;
> }
>
>
> --
> Stefan Richter
> -=====-==--- =--- -=--=
> http://arcgraph.de/sr/
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists