linux-kernel - Re: [PATCH v3] blkcg: allocate struct blkcg

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: <20170304192303.GA29316@wtj.duckdns.org>
Date:   Sat, 4 Mar 2017 11:23:03 -0800
From:   Tejun Heo <tj@...nel.org>
To:     Tahsin Erdogan <tahsin@...gle.com>
Cc:     Jens Axboe <axboe@...nel.dk>, linux-block@...r.kernel.org,
        David Rientjes <rientjes@...gle.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] blkcg: allocate struct blkcg_gq outside request queue
 spinlock

Hello, Tahsin.

Generally looks good.  Please see below.

> @@ -184,24 +185,48 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
> +	if (unlikely(!wb_congested)) {
>  		ret = -ENOMEM;
>  		goto err_put_css;
> +	} else if (unlikely(!blkg)) {
> +		ret = -ENOMEM;
> +		goto err_put_congested;
>  	}

I'm not sure this error handling logic is correct.  We can have any
combo of alloc failure here, right?

> @@ -694,9 +695,20 @@ static inline bool blkcg_bio_issue_check(struct request_queue *q,
>  	blkg = blkg_lookup(blkcg, q);
>  	if (unlikely(!blkg)) {
>  		spin_lock_irq(q->queue_lock);
> -		blkg = blkg_lookup_create(blkcg, q);
> -		if (IS_ERR(blkg))
> -			blkg = NULL;
> +
> +		/*
> +		 * This could be the first entry point of blkcg implementation
> +		 * and we shouldn't allow anything to go through for a bypassing
> +		 * queue.
> +		 */
> +		if (likely(!blk_queue_bypass(q))) {
> +			blkg = blkg_lookup_create(blkcg, q,
> +						  GFP_NOWAIT | __GFP_NOWARN,
> +						  NULL);
> +			if (IS_ERR(blkg))
> +				blkg = NULL;
> +		}

Heh, this seems a bit too fragile.  I get that this covers both call
paths into lookup_create which needs the checking, but it seems a bit
too nasty to do it this way.  Would it be ugly if we factor out both
policy enabled and bypass tests into a helper and have inner
__blkg_lookup_create() which skips it?  We can call the same helper
from blkg_create() when @pol.  Sorry that this is involving so much
bikeshedding.

Thanks!

-- 
tejun