Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 23 Jul 2018 10:13:36 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Anton Vasilyev <vasilyev@ispras.ru>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Samuel Holland <samuel@sholland.org>,
        Pan Bian <bianpan2016@163.com>, linux-kernel@vger.kernel.org,
        ldv-project@linuxtesting.org
Subject: Re: [PATCH] firmware: vpd: Fix section enabled flag on
 vpd_section_destroy
Message-ID: <20180723171336.GA15900@roeck-us.net>
References: <20180723164857.24460-1-vasilyev@ispras.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180723164857.24460-1-vasilyev@ispras.ru>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jul 23, 2018 at 07:48:57PM +0300, Anton Vasilyev wrote:
> static struct ro_vpd and rw_vpd are initialized by vpd_sections_init()
> in vpd_probe() based on header's ro and rw sizes.
> In vpd_remove() vpd_section_destroy() performs deinitialization based
> on enabled flag, which is set to true by vpd_sections_init().
> This leads to call of vpd_section_destroy() on already destroyed section
> for probe-release-probe-release sequence if first probe performs
> ro_vpd initialization and second probe does not initialize it.
> 

I am not sure if the situation described can be seen in the first place.
The second probe would only not perform ro_vpd initialization if it fails
prior to that, ie if it fails to allocate memory or if there is a
consistency problem. In that case the remove function would not be called.

However, there is a problem in the code: A partially failed probe will
leave the system in inconsistent state. Example: ro section initializes,
rw section fails to initialize. The probe will fail, but the ro section
will not be destroyed, its sysfs attributes still exist, and its memory
is still mapped. It would make more sense to fix _that_ problem.
Essentially, vpd_sections_init() should clean up after itself after it
fails to initialize a section.

Note that I am not convinced that the "enabled" flag is needed in the first
place. It is only relevant if vpd_section_destroy() is called, which only
happens from the remove function. The remove function is only called if the
probe function succeeded. In that case it is always set for both sections.

Thanks,
Guenter

> The patch adds changing enabled flag on vpd_section_destroy.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
> ---
>  drivers/firmware/google/vpd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
> index e9db895916c3..5347c17c7108 100644
> --- a/drivers/firmware/google/vpd.c
> +++ b/drivers/firmware/google/vpd.c
> @@ -246,6 +246,7 @@ static int vpd_section_destroy(struct vpd_section *sec)
>  		sysfs_remove_bin_file(vpd_kobj, &sec->bin_attr);
>  		kfree(sec->raw_name);
>  		memunmap(sec->baseaddr);
> +		sec->enabled = false;
>  	}
>  
>  	return 0;
> -- 
> 2.18.0
>