Messages in this thread | | | From | Dragan Cvetic <> | Subject | RE: [RFC PATCH] misc: xilinx-sdfec: Check if file->private_data is NULL | Date | Fri, 21 May 2021 19:25:18 +0000 |
| |
> -----Original Message----- > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Sent: Friday 21 May 2021 19:40 > To: Guenter Roeck <linux@roeck-us.net> > Cc: Derek Kiernan <dkiernan@xilinx.com>; Dragan Cvetic <draganc@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; Michal Simek > <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [RFC PATCH] misc: xilinx-sdfec: Check if file->private_data is NULL > > On Thu, May 20, 2021 at 10:01:50AM -0700, Guenter Roeck wrote: > > container_of() only returns NULL if the passed pointer is NULL _and_ > > the embedded element is the first element of the structure. Even if that > > is the case, testing against it is misleading and possibly dangerous > > because the position of the embedded element may change. Explicitly > > check if the parameter is NULL and bail out if so instead of checking > > the result of container_of(). > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > --- > > RFC: > > > > The NULL check in the poll function is likely unnecessary. Interestingly, > > there is no NULL check in the ioctl function, even though there is a > > similar container_of() in that function. However, I do not feel > > comfortable enough to change the functionality of this code and drop > > the check entirely. > > > > drivers/misc/xilinx_sdfec.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c > > index 23c8448a9c3b..0a3721d31dea 100644 > > --- a/drivers/misc/xilinx_sdfec.c > > +++ b/drivers/misc/xilinx_sdfec.c > > @@ -1011,11 +1011,11 @@ static __poll_t xsdfec_poll(struct file *file, poll_table *wait) > > __poll_t mask = 0; > > struct xsdfec_dev *xsdfec; > > > > - xsdfec = container_of(file->private_data, struct xsdfec_dev, miscdev); > > - > > - if (!xsdfec) > > + if (!file->private_data) > > return EPOLLNVAL | EPOLLHUP; > > > > + xsdfec = container_of(file->private_data, struct xsdfec_dev, miscdev); > > + > > poll_wait(file, &xsdfec->waitq, wait); > > > > /* XSDFEC ISR detected an error */ > > -- > > 2.25.1 > > > > > It should be safe not to check this for NULL as the misc device sets the > pointer in the open, and removes it in release. poll or ioctl can not > be called if release has already happened. > > So feel free to drop the check here, xsdfec_dev_ioctl() looks correct. > > thanks, > > greg k-h
Acked-by: Dragan Cvetic <dragan.cvetic@xilinx.com>
| |