linux-kernel - Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 6 Jul 2016 20:46:57 +0300
From:	Andy Shevchenko <andy.shevchenko@...il.com>
To:	"Franklin S Cooper Jr." <fcooper@...com>
Cc:	david.s.gordon@...el.com, Jens Axboe <axboe@...com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ming Lin <ming.l@....samsung.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Mark Brown <broonie@...nel.org>,
	linux-spi <linux-spi@...r.kernel.org>,
	Sekhar Nori <nsekhar@...com>,
	Peter Ujfalusi <peter.ujfalusi@...com>
Subject: Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist

On Wed, Jul 6, 2016 at 8:09 PM, Franklin S Cooper Jr. <fcooper@...com> wrote:

>>> +               last_entry = sg_is_last(sgl);
>>> +               *sgl = *orig_sgl;
>>> +
>>> +               /*
>>> +                * If page_link is pointing to a chained sgl then set it to
>>> +                * zero since we already compensated for chained sgl. If
>>> +                * page_link is pointing to a page then clear bits 1 and 0.
>>> +                */
>>> +               if (sg_is_chain(orig_sgl))
>>> +                       sgl->page_link = 0x0;
>>> +               else
>>> +                       sgl->page_link &= ~0x03;
>>> +
>>
>>> +               if (last_entry) {
>>> +                       sg_dma_len(sgl) = len;
>>
>> Hmm... If it's not DMA mapped and CONFIG_NEED_SG_DMA_LENGTH=y, you
>> will set dma_length field and len otherwise. Is it on purpose?
>
> Thanks for pointing this out. I didn't take into consideration the above
> scenario.
>
> After looking at this more it seems like on occasion dma_length !=
> length. I see this occurring in various map_sg functions for several
> architectures.
>
> I'm simply trying to reduce the overall sgl length by a certain amount.
> I'm not sure what the proper approach would be since dma_length and
> length can be set to different values. Simply setting sgl->dma_length to
> sgl->length or vice a versa seems like it can be problematic in some
> scenarios. What confuses me even more is if dma_length != length then
> how can sg_nents_for_len only use the sgl length property.
>
> Any thoughts on what would be the best way to handle this?

First come to my mind is to refuse to clone if SG table is DMA mapped.

Imagine the scenario where you have for example last entry which your
caller asked to split as

total_entry_len = 37235 (input)
dma_length = 65536 (64k alignment)

asked_len = 32768 (Split chunks: 32768 + 4467)
resulting_dma_len = ?

What do you put here? Initially it perhaps means the same 64k. But how
to distinguish?
(I couldn't come with better one, but there are many alike scenarios)

Only hack we have is to supply struct device of the DMA device to this
SG table where you can get max segment size (but I dunno about
alignment).

P.S. I'm not familiar of the details of all these.

-- 
With Best Regards,
Andy Shevchenko