________________________________________
From: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Sent: Thursday, April 30, 2015 12:45 PM
To: Gujulan Elango, Hari Prasath (H.)
Cc: devel@xxxxxxxxxxxxxxxxxxxx; Julia.Lawall@xxxxxxx; florian.c.schilhabel@xxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; Babu, Viswanathan (V.); Larry.Finger@xxxxxxxxxxxx
Subject: Re: [PATCHv2] staging: rtl8712: Fix freeing ERR_PTR
I sent a fix for this a couple weeks ago.
On Thu, Apr 30, 2015 at 06:32:01AM +0000, Gujulan Elango, Hari Prasath (H.) wrote:
> The return value of memdup_user is a pointer to errno.Freeing it will cause
> error.Hence set it to NULL before branching to free the pointer.smatch also
> raises the same warning.
>
> Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@xxxxxxxxxxx>
> ---
> v2:Remove unnecessary goto and return error directly.Use goto
> only if some cleanup is done.These were review comments by
> Julia Lawall.
> ---
> drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 24 +++++++-----------------
> 1 file changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> index 42fba3f..f563b2d 100644
> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> @@ -1905,18 +1905,14 @@ static int r871x_mp_ioctl_hdl(struct net_device *dev,
> uint status;
> int ret = 0;
>
> - if ((!p->length) || (!p->pointer)) {
> - ret = -EINVAL;
> - goto _r871x_mp_ioctl_hdl_exit;
> - }
> + if ((!p->length) || (!p->pointer))
> + return -EINVAL;
> bset = (u8)(p->flags & 0xFFFF);
> len = p->length;
> pparmbuf = NULL;
> pparmbuf = memdup_user(p->pointer, len);
> - if (IS_ERR(pparmbuf)) {
> - ret = PTR_ERR(pparmbuf);
> - goto _r871x_mp_ioctl_hdl_exit;
> - }
> + if (IS_ERR(pparmbuf))
> + return PTR_ERR(pparmbuf);
These changes are good. My patch was similar.
http://www.spinics.net/lists/linux-kernel-janitors/msg21846.html
> poidparam = (struct mp_ioctl_param *)pparmbuf;
> if (poidparam->subcode >= MAX_MP_IOCTL_SUBCODE) {
> ret = -EINVAL;
> @@ -2058,21 +2054,15 @@ static int r871x_set_chplan(struct net_device *dev,
> struct iw_request_info *info,
> union iwreq_data *wrqu, char *extra)
> {
> - int ret = 0;
> struct _adapter *padapter = netdev_priv(dev);
> struct iw_point *pdata = &wrqu->data;
> int ch_plan = -1;
>
> - if ((padapter->bDriverStopped) || (pdata == NULL)) {
> - ret = -EINVAL;
> - goto exit;
> - }
> + if ((padapter->bDriverStopped) || (pdata == NULL))
> + return -EINVAL;
> ch_plan = (int)*extra;
> r8712_set_chplan_cmd(padapter, ch_plan);
> -
> -exit:
> -
> - return ret;
> + return 0;
These changes are an unrelated cleanup to a different function. Only do
one thing per patch.
---
Ok Dan I would send patch v3 with only these changes. I will send the cleanup done on another unrelated function as a separate patch.
regards
Hari Prasath
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
