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

[Phillip Potter]

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. 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, 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.

Regards,
Phil