linux-kernel - Re: [PATCH 2/4] direct-io: make O

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: <20090820171204.93c3fdb4.akpm@linux-foundation.org>
Date:	Thu, 20 Aug 2009 17:12:04 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Jens Axboe <jens.axboe@...cle.com>
Cc:	linux-kernel@...r.kernel.org, jeff@...zik.org,
	benh@...nel.crashing.org, htejun@...il.com, bzolnier@...il.com,
	alan@...rguk.ukuu.org.uk, jens.axboe@...cle.com
Subject: Re: [PATCH 2/4] direct-io: make O_DIRECT IO path be page based

On Thu, 20 Aug 2009 12:17:38 +0200
Jens Axboe <jens.axboe@...cle.com> wrote:

> Currently we pass in the iovec array and let the O_DIRECT core
> handle the get_user_pages() business. This work, but it means that
> we can ever only use user pages for O_DIRECT.
> 
> Switch the aops->direct_IO() and below code to use page arrays
> instead, so that it doesn't make any assumptions about who the pages
> belong to. This works directly for all users but NFS, which just
> uses the same helper that the generic mapping read/write functions
> also call.
>

This:

> +	struct page **pages;		/* page buffer */
> +	unsigned int head_page;		/* next page to process */
> +	unsigned int total_pages;	/* last valid page + 1 */
> +	unsigned int first_page_off;	/* offset into first page in map */

Is a disaster waiting to happen.

>  static struct page *dio_get_page(struct dio *dio)
>  {
> -	if (dio_pages_present(dio) == 0) {
> -		int ret;
> +	if (dio->head_page < dio->total_pages)
> +		return dio->pages[dio->head_page++];
>  
> -		ret = dio_refill_pages(dio);
> -		if (ret)
> -			return ERR_PTR(ret);
> -		BUG_ON(dio_pages_present(dio) == 0);
> -	}
> -	return dio->pages[dio->head++];
> +	return NULL;
>  }
>
> ...
>
> @@ -351,8 +280,10 @@ static void dio_bio_submit(struct dio *dio)
>   */
>  static void dio_cleanup(struct dio *dio)
>  {
> -	while (dio_pages_present(dio))
> -		page_cache_release(dio_get_page(dio));
> +	struct page *page;
> +
> +	while ((page = dio_get_page(dio)) != NULL)
> +		page_cache_release(page);
>  }

What is the value of dio->head_page here?

How are callers to use this function at the start of the operation, at
the end and partway through is IO error, EFAULT etc is detected?

There's good potential for double-releases, leaks etc here.  We need to
be very careful and explicit in documenting the usage and state and
meaning of head_page, and the protocol around managing it.

It's just like that ghastly bio.bi_idx thing - how many bugs did we add
and have to fix due to that thing?

Really I think it'd be better to just nuke .head_page altogether and get
callers to manage their index openly.

--
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/