linux-kernel - Re: [PATCH V2] nvme-pci: fix NULL pointer reference in nvme_alloc

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:   Thu, 4 Jan 2018 11:20:33 +0100
From:   Christoph Hellwig <hch@....de>
To:     Jianchao Wang <jianchao.w.wang@...cle.com>
Cc:     keith.busch@...el.com, axboe@...com, hch@....de, sagi@...mberg.me,
        linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] nvme-pci: fix NULL pointer reference in
        nvme_alloc_ns

This looks generally fine to me, ut a few nitpicks below:

>  - Based on Sagi's suggestion, add new state NVME_CTRL_ADMIN_LIVE.

Maybe call this NVME_CTRL_ADMIN_ONLY ?

> -	if (ctrl->state != NVME_CTRL_LIVE)
> +	if ((ctrl->state != NVME_CTRL_LIVE) &&
> +		(ctrl->state != NVME_CTRL_ADMIN_LIVE))

No need for the inner braces, and odd indentation.  Also in general
I'm tempted to just use switch statements for things like this, e.g.

	switch (ctrl->state) {
	case NVME_CTRL_ADMIN_LIVE:
	case NVME_CTRL_LIVE:
		break;
	default:
		return -EWOULDBLOCK;
	}

> @@ -3074,6 +3087,8 @@ static void nvme_scan_work(struct work_struct *work)
>  	if (ctrl->state != NVME_CTRL_LIVE)
>  		return;
>  
> +	BUG_ON(!ctrl->tagset);

WARN_ON_ONCE() please.

> +	bool only_adminq = false;

How about a new_state variable instead that holds the new state value?