Re: [PATCH 2/3] staging: r8188eu: simplify c2h_evt_hdl function

[Fabio M. De Francesco]

On Sunday, August 29, 2021 12:49:42 PM CEST Phillip Potter wrote:
> On Sun, 29 Aug 2021 at 09:52, Fabio M. De Francesco
> <fmdefrancesco@xxxxxxxxx> wrote:
> >
> > >  static s32 c2h_evt_hdl(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt, c2h_id_filter filter)
> > >  {
> > > -     s32 ret = _FAIL;
> > >       u8 buf[16];
> > >
> > > -     if (!c2h_evt) {
> > > -             /* No c2h event in cmd_obj, read c2h event before handling*/
> > > -             if (c2h_evt_read(adapter, buf) == _SUCCESS) {
> > > -                     c2h_evt = (struct c2h_evt_hdr *)buf;
> >
> > Dear Philip,
> >
> > Not related to your patch, but what kind of odd assignment is it? c2h_evt takes
> > the address of a local variable and therefore it crashes the kernel whenever
> > someone decides to dereference it after this function returns and unwinds
> > the stack...
> 
> Dear Fabio,
> 
> Thank you for taking a look firstly, really appreciate it :-) As for the line:
> c2h_evt = (struct c2h_evt_hdr *)buf;
> 
> in the original code before I removed it, bear in mind that this
> pointer assignment is
> happening into the parameter variable c2h_evt, which is a copy of the
> passed in argument,
> as C is pass-by-value. 

You're right! Sorry. For sure I was still sleeping when I wrote that message. 
The assignment is to the argument itself, not to the storage 
location it points to... for a moment I forgot how arguments are passed
(by value) and how pointers work :(

> Therefore, after the c2h_evt_hdl function
> returns, the value passed
> in as the argument for this parameter would still have its original
> pointer value (or NULL).
> 
> It would not therefore be possible to deference the pointer to this
> stack-allocated memory
> from outside the function, even in the original code. I agree though,
> its purpose is dubious.
> Originally, the wrapper function rtw_hal_c2h_handler would have passed
> it through to the
> function assigned to the c2h_handler function pointer, but there was
> no such function in
> this driver, so it was never executed.
> 
> >
> > > +     if (!c2h_evt)
> > > +             c2h_evt_read(adapter, buf);
> >
> > Having said that, I strongly doubt that this path is ever taken. I didn't check the call
> > chain, but it may be that the function in never called or, if it is called, it always
> > has a valid c2h_evt argument.
> >
> > Actually I don't mean to suggest something specific. It simply looks odd, so I'd check
> > and if this happens to be the case, I'd remove the whole c2h_evt_hdl().
> >
> > Regards,
> >
> > Fabio
> 
> As alluded to, removing the whole of c2h_evt_hdl would lead to
> c2h_evt_read no longer
> being executed, 

Yes, correct. I see that the only caller of c2h_evt_hdl() is at line 1971 of  core/rtw_cmd.c:
"c2h_evt_hdl(padapter, (struct c2h_evt_hdr *)pdrvextra_cmd->pbuf, NULL);".
Actually I cannot say whether or not pdrvextra_cmd->pbuf is properly initialized at
that specific point.

Maybe that staying on the safe side and checking for !c2h_evt is the most reasonable
thing to do...

Some time ago, Greg wrote that even reads can have side effects on some (broken?) 
hardware. So, you're on the safe side if you leave that read where it is. Well done. 

> which would mean the reads from the adapter register
> don't happen, and
> nor does the clearing by c2h_evt_clear(adapter); - in particular, the
> comment there mentions
> the FW not updating the next command message if this isn't executed when needed.
> 
> This may be perfectly fine, but I thought this approach is safer due
> to the above.

Yes, I must agree with you :)

Thanks for making me notice that detail about arguments. Maybe that that odd 
assignment made me forget for a moment how passing arguments works in C.

Thanks,

Fabio

> Regards,
> Phil
>