On Sunday, August 22, 2021 12:09:29 PM CEST Pavel Skripkin wrote:

On 8/22/21 12:53 PM, Fabio M. De Francesco wrote:

quoted

On Friday, August 20, 2021 7:07:28 PM CEST Pavel Skripkin wrote:

quoted

Hi, Greg, Larry and Phillip!

I noticed, that new staging driver was added like 3 weeks ago and I 

decided

quoted

quoted

to look at the code, because drivers in staging directory are always 

buggy.

quoted

quoted

The first thing I noticed is *no one* was checking read operations 

result,

quoted

but

quoted

it can fail and driver may start writing random stack values into 

registers.

quoted

It

quoted

can cause driver misbehavior or device misbehavior.

After the messages I wrote yesterday, I had some minutes to look deeper at 

the

quoted

code that would be changed by these patches.

I think that it does not look like that the driver could return "random 

stack

quoted

values into registers" and I think this entire series in unnecessary.

As far as I understand this driver (though I must admit that I really 

don't

quoted

know how to write drivers, and I'm not interested in understanding - at 

the

quoted

moment, at least), all the usb_read*() call usbctrl_vendorreq() and the 

latter

quoted

*does* proper error checking before returning to the callers the read 

data.

quoted

Please, look at the code copied from usbctrl_vendorreq() and pasted here 

(some

quoted

comments are mine):

/* start of code */
static int usbctrl_vendorreq(struct intf_hdl *pintfhdl, u16 value, void
*pdata, u16 len, u8 requesttype)
{

/* test if everything is OK for transfers and setup the necessary 

variables */

quoted

[...]

status = usb_control_msg(udev, pipe, REALTEK_USB_VENQT_CMD_REQ,

                                          reqtype, value,

REALTEK_USB_VENQT_CMD_IDX,

                                          pIo_buf, len,

RTW_USB_CONTROL_MSG_TIMEOUT);

                 if (status == len) {   /*  Success this control transfer. 

*/

quoted

                 
                         rtw_reset_continual_urb_error(dvobjpriv);
                         if (requesttype == 0x01)
                         
                                 memcpy(pdata, pIo_buf,  len); /* pdata

receives the read data */

	} else { /*  error cases */

[...]

}
/* end of code */

So, *I cannot ack this RFC*, unless maintainers say I'm missing something.

Larry, Philip, since you have much more knowledge than me about r8188eu 

(and,

quoted

more in general, on device drivers) may you please say what you think 

about my

quoted

arguments against this series?

Hi, Fabio!

Thank you for looking into this, but I still can see the case when pdata
won't be initialized:


pdata is initialized only in case of successful transfer, i.e len > 0.
It means some data was received (maybe not full length, but anyway). In
case of usb_control_msg() error (for example -ENOMEM) code only does
this code block:

if (status < 0) {
	if (status == (-ESHUTDOWN) || status == -ENODEV) {
		adapt->bSurpriseRemoved = true;
	} else {
		struct hal_data_8188e	*haldata = GET_HAL_DATA(adapt);
		haldata->srestpriv.Wifi_Error_Status = 

USB_VEN_REQ_CMD_FAIL;

	}
}

It's up to the callers of _rtw_usb*() to check return values and then act 
accordingly. 

It doesn't matter whether or not *pdata is initialized because usb_read*() 
returns data = 0 if usb_control_msg() has not initialized/changed  its third 
parameter. Then _rtw_read*() receive 0 or initialized data depending on errors 
or no errors. Finally _rtw_read*() returns that same value to the callers (via 
r_val). 

So, it's up to the callers to test if (!_rtw_read*()) and then act 
accordingly. If they get 0 they should know how to handle the errors.

Furthermore, we have already either adapt->bSurpriseRemoved = true or haldata-

srestpriv.Wifi_Error_Status = USB_VEN_REQ_CMD_FAIL. Depending on contexts 

where _rtw_read*() are called, perhaps they could also check the two variables 
above.

In summation. if anything should be changed, it is the code of the callers of 
_rtw_read*() if you find out they they don't properly handle the returning 
values of this function. You should find every place where _rtw_read*() are 
called and figure out if the returns are properly checked and handled; if not, 
make some change only there.

Larry, Philip, where are you? Am I missing something?

Thanks,

Fabio

And then just loops further. In case of 10 ENOMEM in a row,. passed
pdata won't be initialized at all and driver doesn't do anything about
it. I believe, it's not good approach to play with random values. We
should somehow handle transfer errors all across the driver.

If I am missing something, please, let me know :)



With regards,
Pavel Skripkin