| Message ID | 20230325083429.3571917-1-harperchen1110@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wireless: rtlwifi: fix incorrect error codes in rtl_debugfs_set_write_rfreg() | expand |
On Sat, Mar 25, 2023 at 08:34:29AM +0000, Wei Chen wrote: > If there is a failure during copy_from_user or user-provided data buffer > is invalid, rtl_debugfs_set_write_rfreg should return negative error code > instead of a positive value count. > > Fix this bug by returning correct error code. Moreover, the check of buffer > against null is removed since it will be handled by copy_from_user. > > Signed-off-by: Wei Chen <harperchen1110@gmail.com> Hi Wei Chen, * I'm not sure if a fixes tag is appropriate for this. But if so, perhaps it should be: Fixes: 610247f46feb ("rtlwifi: Improve debugging by using debugfs") * I think the preferred subject prefix may be 'rtlwifi: ' (without the leading 'wireless: '). * This seems to be v2 of this patch, which would be best noted in the subject '[PATCH v2]'. The above notwithstanding, the code changes look correct to me. Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > drivers/net/wireless/realtek/rtlwifi/debug.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtlwifi/debug.c b/drivers/net/wireless/realtek/rtlwifi/debug.c > index 3e7f9b4f1f19..9eb26dfe4ca9 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/debug.c > +++ b/drivers/net/wireless/realtek/rtlwifi/debug.c > @@ -375,8 +375,8 @@ static ssize_t rtl_debugfs_set_write_rfreg(struct file *filp, > > tmp_len = (count > sizeof(tmp) - 1 ? sizeof(tmp) - 1 : count); > > - if (!buffer || copy_from_user(tmp, buffer, tmp_len)) > - return count; > + if (copy_from_user(tmp, buffer, tmp_len)) > + return -EFAULT; > > tmp[tmp_len] = '\0'; > > @@ -386,7 +386,7 @@ static ssize_t rtl_debugfs_set_write_rfreg(struct file *filp, > if (num != 4) { > rtl_dbg(rtlpriv, COMP_ERR, DBG_DMESG, > "Format is <path> <addr> <mask> <data>\n"); > - return count; > + return -EINVAL; > } > > rtl_set_rfreg(hw, path, addr, bitmask, data); > -- > 2.25.1 >
Dear Simon, Thanks for the advice and review. I have sent the second version of the patch. Besides, rtl_debugfs_set_write_reg also suffers from the incorrect error code problem. I also sent v2 of the corresponding patch. Hope there is no confusion between these two patches. Best, Wei On Sat, 25 Mar 2023 at 17:48, Simon Horman <simon.horman@corigine.com> wrote: > > On Sat, Mar 25, 2023 at 08:34:29AM +0000, Wei Chen wrote: > > If there is a failure during copy_from_user or user-provided data buffer > > is invalid, rtl_debugfs_set_write_rfreg should return negative error code > > instead of a positive value count. > > > > Fix this bug by returning correct error code. Moreover, the check of buffer > > against null is removed since it will be handled by copy_from_user. > > > > Signed-off-by: Wei Chen <harperchen1110@gmail.com> > > Hi Wei Chen, > > * I'm not sure if a fixes tag is appropriate for this. > But if so, perhaps it should be: > > Fixes: 610247f46feb ("rtlwifi: Improve debugging by using debugfs") > > * I think the preferred subject prefix may be 'rtlwifi: ' (without the leading 'wireless: '). > > * This seems to be v2 of this patch, which would be best noted in > the subject '[PATCH v2]'. > > The above notwithstanding, the code changes look correct to me. > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > > > --- > > drivers/net/wireless/realtek/rtlwifi/debug.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtlwifi/debug.c b/drivers/net/wireless/realtek/rtlwifi/debug.c > > index 3e7f9b4f1f19..9eb26dfe4ca9 100644 > > --- a/drivers/net/wireless/realtek/rtlwifi/debug.c > > +++ b/drivers/net/wireless/realtek/rtlwifi/debug.c > > @@ -375,8 +375,8 @@ static ssize_t rtl_debugfs_set_write_rfreg(struct file *filp, > > > > tmp_len = (count > sizeof(tmp) - 1 ? sizeof(tmp) - 1 : count); > > > > - if (!buffer || copy_from_user(tmp, buffer, tmp_len)) > > - return count; > > + if (copy_from_user(tmp, buffer, tmp_len)) > > + return -EFAULT; > > > > tmp[tmp_len] = '\0'; > > > > @@ -386,7 +386,7 @@ static ssize_t rtl_debugfs_set_write_rfreg(struct file *filp, > > if (num != 4) { > > rtl_dbg(rtlpriv, COMP_ERR, DBG_DMESG, > > "Format is <path> <addr> <mask> <data>\n"); > > - return count; > > + return -EINVAL; > > } > > > > rtl_set_rfreg(hw, path, addr, bitmask, data); > > -- > > 2.25.1 > >
On Sun, Mar 26, 2023 at 01:47:51PM +0800, Wei Chen wrote: > Dear Simon, > > Thanks for the advice and review. I have sent the second version of the patch. > > Besides, rtl_debugfs_set_write_reg also suffers from the incorrect > error code problem. I also sent v2 of the corresponding patch. Hope > there is no confusion between these two patches. Thanks. I now see there are two similar but different patches. My bad.
On 3/26/23 03:03, Simon Horman wrote: > On Sun, Mar 26, 2023 at 01:47:51PM +0800, Wei Chen wrote: >> Dear Simon, >> >> Thanks for the advice and review. I have sent the second version of the patch. >> >> Besides, rtl_debugfs_set_write_reg also suffers from the incorrect >> error code problem. I also sent v2 of the corresponding patch. Hope >> there is no confusion between these two patches. > > Thanks. I now see there are two similar but different patches. My bad. Avoid all such misunderstandings by making a patch set. Larry
diff --git a/drivers/net/wireless/realtek/rtlwifi/debug.c b/drivers/net/wireless/realtek/rtlwifi/debug.c index 3e7f9b4f1f19..9eb26dfe4ca9 100644 --- a/drivers/net/wireless/realtek/rtlwifi/debug.c +++ b/drivers/net/wireless/realtek/rtlwifi/debug.c @@ -375,8 +375,8 @@ static ssize_t rtl_debugfs_set_write_rfreg(struct file *filp, tmp_len = (count > sizeof(tmp) - 1 ? sizeof(tmp) - 1 : count); - if (!buffer || copy_from_user(tmp, buffer, tmp_len)) - return count; + if (copy_from_user(tmp, buffer, tmp_len)) + return -EFAULT; tmp[tmp_len] = '\0'; @@ -386,7 +386,7 @@ static ssize_t rtl_debugfs_set_write_rfreg(struct file *filp, if (num != 4) { rtl_dbg(rtlpriv, COMP_ERR, DBG_DMESG, "Format is <path> <addr> <mask> <data>\n"); - return count; + return -EINVAL; } rtl_set_rfreg(hw, path, addr, bitmask, data);
If there is a failure during copy_from_user or user-provided data buffer is invalid, rtl_debugfs_set_write_rfreg should return negative error code instead of a positive value count. Fix this bug by returning correct error code. Moreover, the check of buffer against null is removed since it will be handled by copy_from_user. Signed-off-by: Wei Chen <harperchen1110@gmail.com> --- drivers/net/wireless/realtek/rtlwifi/debug.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)