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 <l...@intel.com>
> Reported-by: Dan Carpenter <dan.carpen...@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-...@lists.01.org
>
