Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1539337 > unrolled thread

scatterwalk_map_and_copy incorrect optimization

Back to article view | Back to linux.kernel

Contents

  scatterwalk_map_and_copy incorrect optimization "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-12-09 14:20 +0100
    Re: scatterwalk_map_and_copy incorrect optimization "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-12-09 14:30 +0100
      Re: scatterwalk_map_and_copy incorrect optimization Herbert Xu <herbert@gondor.apana.org.au> - 2016-12-10 09:20 +0100
    Re: scatterwalk_map_and_copy incorrect optimization Herbert Xu <herbert@gondor.apana.org.au> - 2016-12-10 09:10 +0100

#1539337 — scatterwalk_map_and_copy incorrect optimization

Hi Herbert,

The scatterwalk_map_and_copy function copies ordinary buffers to and
from scatterlists. These buffers can, of course, be on the stack, and
this remains the most popular use of this function -- getting info
between stack buffers and DMA regions. It's mostly used for adding or
checking MACs, in the majority of call sites. Its implementation is
relatively straightforward. It maps the DMA region(s) to a vaddr, and
then just calls vanilla memcpy. Pretty uncontroversial.

However, around ~4.1 an optimization was added to prevent copying when
unnecessary (when the src and dst are the same). The optimization
looks like this:

        if (sg_page(sg) == virt_to_page(buf) &&
           sg->offset == offset_in_page(buf))
               return;

There are two problems with this:

1) If buf points to a large contiguous region, but sg points to
several smaller regions, with the first one of which being smaller
than buf, then this function will not actually copy the latter
fragments to the large contiguous buf. Maybe you don't care about
this, but it is a limitation and a potential source of bugs down the
line.

2) If buf points to the stack, this optimization is totally wrong,
since you shouldn't call virt_to_page on stack addresses.

Since this function is primarily used for copying to and from the
stack, item (2) is especially worrisome. A very quick fix for that
would be to just:

        if (!object_is_on_stack(buf) &&
           sg_page(sg) == virt_to_page(buf) &&
           sg->offset == offset_in_page(buf))
               return;

This doesn't address item (1), however. If you care about fixing item
(1), then maybe a reasonable way would be to just remove the
optimization all together.

I'll submit a patch for the !object_is_on_stack(buf) fix. But maybe
you'd prefer that I submit a patch that removes the whole optimization
entirely. Or something else -- just let me know.

Regards,
Jason

[toc] | [next] | [standalone]

#1539344

Hah, looks like I missed [1] by a couple weeks. Looks like it's been
settled then.

Is this a stable@ candidate?


[1] https://git.zx2c4.com/linux/commit/?id=c8467f7a3620698bf3c22f0e199b550fb611a8ae

[toc] | [prev] | [next] | [standalone]

#1539797

Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Hah, looks like I missed [1] by a couple weeks. Looks like it's been
> settled then.
> 
> Is this a stable@ candidate?

Not really since it shouldn't cause any problems unless the stack
is vmalloced.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[toc] | [prev] | [next] | [standalone]

#1539794

On Fri, Dec 09, 2016 at 02:18:01PM +0100, Jason A. Donenfeld wrote:
> Hi Herbert,
> 
> The scatterwalk_map_and_copy function copies ordinary buffers to and
> from scatterlists. These buffers can, of course, be on the stack, and
> this remains the most popular use of this function -- getting info
> between stack buffers and DMA regions. It's mostly used for adding or
> checking MACs, in the majority of call sites. Its implementation is
> relatively straightforward. It maps the DMA region(s) to a vaddr, and
> then just calls vanilla memcpy. Pretty uncontroversial.
> 
> However, around ~4.1 an optimization was added to prevent copying when
> unnecessary (when the src and dst are the same). The optimization
> looks like this:
> 
>         if (sg_page(sg) == virt_to_page(buf) &&
>            sg->offset == offset_in_page(buf))
>                return;
> 
> There are two problems with this:

This code no longer exists in the current tree.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web