Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4 14/16] rpmsg: char: introduce a RPMsg driver for the
 RPMsg char device
To:     Dan Carpenter <dan.carpenter@oracle.com>, <kbuild@lists.01.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Ohad Ben-Cohen <ohad@wizery.com>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Andy Gross <agross@kernel.org>
CC:     <lkp@intel.com>, <kbuild-all@lists.01.org>,
        <linux-remoteproc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>,
        <linux-arm-msm@vger.kernel.org>
References: <20210218123333.GK2087@kadam>
From:   Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Message-ID: <dbfb7ced-bc0c-5e47-a937-3fe07a4129bf@foss.st.com>
Date:   Thu, 18 Feb 2021 18:52:43 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20210218123333.GK2087@kadam>
Content-Type: text/plain; charset="windows-1252"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Dan,

On 2/18/21 1:33 PM, Dan Carpenter wrote:
> Hi Arnaud,
> 
> url:    https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-a-generic-IOCTL-interface-for-RPMsg-channels-management/20210217-214044
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
> config: riscv-randconfig-m031-20210215 (attached as .config)
> compiler: riscv32-linux-gcc (GCC) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> drivers/rpmsg/rpmsg_char.c:429 rpmsg_chrdev_probe() error: we previously assumed 'rpdev->ept' could be null (see line 423)
> 
> vim +429 drivers/rpmsg/rpmsg_char.c
> 
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  413  static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  414  {
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  415  	struct rpmsg_channel_info chinfo;
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  416  	struct rpmsg_eptdev *eptdev;
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  417  
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  418  	memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  419  	chinfo.src = rpdev->src;
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  420  	chinfo.dst = rpdev->dst;
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  421  
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  422  	eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 @423  	if (IS_ERR(eptdev) && rpdev->ept) {
>                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This condition is strange.

> 
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  424  		rpmsg_destroy_ept(rpdev->ept);
>                                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> What?  Why are we undoing this when it's not something that we created?
> This seems like a layering violation...

Right,something is not clean here, I need to crosscheck, but should be
	if (IS_ERR(eptdev) && ) {
		return PTR_ERR(eptdev);
	}
The endpoint is already destroyed by rpmsg_dev_probe on error.

> 
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  425  		return PTR_ERR(eptdev);
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  426  	}
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  427  
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  428  	/* Set the private field of the default endpoint to retrieve context on callback. */
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17 @429  	rpdev->ept->priv = eptdev;
>                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^
> If "rpdev->ept" is NULL this will Oops.  If "eptdev" is an error pointer
> that seems wrong as well.

rpdev->ept is set in rpmsg_dev_probe as the callback is defined so can not be
null, so probably a false positive here.
eptdev can not be an error pointer here for the same reason.

Anyway adding a check on the pointer, is not a big work and can prevent from
future issue.

As consequence of you multi-reports I have installed your smatch tool on my PC
and added it in my compilation chain. :)

Thanks for the review and the tool,
Arnaud

> 
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  430  
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  431  	return 0;
> 7337f30f7a4426 Arnaud Pouliquen 2021-02-17  432  }
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>