linux-kernel - Re: [PATCH rdma-next v2 1/2] lib/scatterlist: Fix wrong update of orig

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]

Message-ID: <20210722130040.GH1117491@nvidia.com>
Date:   Thu, 22 Jul 2021 10:00:40 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     Doug Ledford <dledford@...hat.com>,
        Christoph Hellwig <hch@...radead.org>,
        Maor Gottlieb <maorg@...dia.com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>,
        dri-devel@...ts.freedesktop.org, intel-gfx@...ts.freedesktop.org,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-rdma@...r.kernel.org,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Mike Marciniszyn <mike.marciniszyn@...nelisnetworks.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Roland Scheidegger <sroland@...are.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        VMware Graphics <linux-graphics-maintainer@...are.com>,
        Yishai Hadas <yishaih@...dia.com>,
        Zack Rusin <zackr@...are.com>,
        Zhu Yanjun <zyjzyj2000@...il.com>
Subject: Re: [PATCH rdma-next v2 1/2] lib/scatterlist: Fix wrong update of
 orig_nents

On Sun, Jul 18, 2021 at 02:09:12PM +0300, Leon Romanovsky wrote:
> @@ -386,12 +414,14 @@ static struct scatterlist *get_next_sg(struct sg_table *table,
>  		return ERR_PTR(-ENOMEM);
>  	sg_init_table(new_sg, alloc_size);
>  	if (cur) {
> +		if (total_nents)
> +			*total_nents += alloc_size - 1;
>  		__sg_chain(next_sg, new_sg);
> -		table->orig_nents += alloc_size - 1;
>  	} else {
>  		table->sgl = new_sg;
> -		table->orig_nents = alloc_size;
>  		table->nents = 0;

Why does this still touch nents?

> @@ -515,6 +548,7 @@ struct scatterlist *__sg_alloc_table_from_pages(struct sg_table *sgt,
>  		cur_page = j;
>  	}
>  	sgt->nents += added_nents;
> +	sgt->orig_nents = sgt->nents;

And here too?

nents should only be set by the dma mapper, right?


I'm also trying to understand why it is OK to pass in NULL for
total_nents?

Any situation where _sg_alloc_table_from_pages() returns with
sgt->orig_nents != total_nents requires the use of
sg_free_table_entries()

It looks like there is some trouble here:

	for (i = 0; i < chunks; i++) {
		s = get_next_sg(sgt, s, chunks - i + left_pages, gfp_mask,
				total_nents);
		if (IS_ERR(s)) {

This will update total_nents but after a few loops it can exit without
synchronizing sgt->orig_nents - thus any caller handling an error
return from __sg_alloc_table_from_pages() must not pass in NULL and
must use sg_free_table_entries()

So I would see two options:

 1) Remove the possiblity to return NULL and fix all callers to use
    sg_free_table_entries() on error

 2) Once __sg_alloc_table_from_pages() fails the sg_table is corrupted
    and the user must call sg_free_table_entries().
    ie forcibly store total_nents in the orig_nents and thus destroy
    the ability to continue to use the sg_table.

    This is what sg_alloc_table_from_pages() already has to do

Further upon success of __sg_alloc_table_from_pages() it should be
true that sgt->orig_nents == total_nents so the ib_umem change is
confusing. total_nents should be removed from the struct and only the
failure paths in the function calling __sg_alloc_table_from_pages()
need a stack local variable and sg_free_table_entries()

IMHO this API may have become unwieldly and complicated, I wonder if
this is better:

   struct sg_append_table state;

   sg_append_init(&state, sgt, gfp_mask);

   while (..)
     ret = sg_append_pages(&state, pages, n_pages, ..)
     if (ret)
	 sg_append_abort(&state); // Frees the sgt and puts it to NULL
   sg_append_complete(&state)

Which allows sg_alloc_table_from_pages() to be written as

   struct sg_append_table state;
   sg_append_init(&state, sgt, gfp_mask);
   ret = sg_append_pages(&state,pages, n_pages, offset, size, UINT_MAX)
   if (ret) {
      sg_append_abort(&state);
      return ret;
   }
   sg_append_complete(&state);
   return 0;

And then the API can manage all of this in some sane and
understandable way.

Jason