linux-kernel - Re: [PATCH v5 1/3] staging: rtl8723bs: change return type of _rtw_pktfile

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aXG3DSMRf1Ojn436@stanley.mountain>
Date: Thu, 22 Jan 2026 08:35:09 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Minu Jin <s9430939@...er.com>
Cc: gregkh@...uxfoundation.org, andriy.shevchenko@...ux.intel.com,
	abrahamadekunle50@...il.com, zxcv2569763104@...il.com,
	milospuric856@...il.com, karanja99erick@...il.com,
	weibu@...admin.org, linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/3] staging: rtl8723bs: change return type of
 _rtw_pktfile_read to int

On Thu, Jan 22, 2026 at 01:14:48PM +0900, Minu Jin wrote:
> The current return type of _rtw_pktfile_read() is uint,
> which makes it impossible to propagate negative error codes
> from internal calls (skb_copy_bits()).
> 
> In preparation for returning proper error codes
> (eg, skb_copy_bits return -EFAULT when error occurs)
> when data is insufficient or copying fails, change the function's
> return type to int.
> 
> Additionally, update the type of the 'rlen' parameter, 'len' variable
> from 'uint' to 'unsigned int' to comply with the kernel coding style
> as suggested by Andy Shevchenko.
> 
> Signed-off-by: Minu Jin <s9430939@...er.com>
> ---
>  drivers/staging/rtl8723bs/include/xmit_osdep.h |  2 +-
>  drivers/staging/rtl8723bs/os_dep/xmit_linux.c  | 14 +++++++++-----
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/include/xmit_osdep.h b/drivers/staging/rtl8723bs/include/xmit_osdep.h
> index 8704dced593a..880344bffe2f 100644
> --- a/drivers/staging/rtl8723bs/include/xmit_osdep.h
> +++ b/drivers/staging/rtl8723bs/include/xmit_osdep.h
> @@ -35,7 +35,7 @@ void rtw_os_xmit_resource_free(struct adapter *padapter, struct xmit_buf *pxmitb
>  
>  extern uint rtw_remainder_len(struct pkt_file *pfile);
>  extern void _rtw_open_pktfile(struct sk_buff *pkt, struct pkt_file *pfile);
> -extern uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen);
> +int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, unsigned int rlen);
>  extern signed int rtw_endofpktfile(struct pkt_file *pfile);
>  
>  extern void rtw_os_pkt_complete(struct adapter *padapter, struct sk_buff *pkt);
> diff --git a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> index 944b9c724b32..ea54a573e025 100644
> --- a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> +++ b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> @@ -21,15 +21,19 @@ void _rtw_open_pktfile(struct sk_buff *pktptr, struct pkt_file *pfile)
>  	pfile->cur_buffer = pfile->buf_start;
>  }
>  
> -uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen)
> +int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, unsigned int rlen)
>  {
> -	uint	len = 0;
> +	unsigned int len;
> +	int ret;
>  
>  	len =  rtw_remainder_len(pfile);
> -	len = (rlen > len) ? len : rlen;
> +	len = (rlen > len) ? len : rlen;

How did you generate this diff?  The before and after lines are
exactly the same.

>  
> -	if (rmem)
> -		skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, len);
> +	if (rmem) {
> +		ret = skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, len);
> +		if (ret < 0)
> +			return ret;

No, you can't do this or it will cause a bug in rtw_xmitframe_coalesce().
So that also means don't change the return type from uint to int in this
patch either.

Normally, we would do the bugfix first and then the cleanup in the
follow on patches.  That way we could backport the fix and leave the
cleanup.  It doesn't matter in a practical sense in this code but
that's the better way.

regards,
dan carpenter

> +	}
>  
>  	pfile->cur_addr += len;
>  	pfile->pkt_len -= len;
> -- 
> 2.43.0