Re: [PATCH v2] CIFS: Fix a possible memory corruption during reconnect

[Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Wed, 2016-11-30 at 11:24 -0800, Pavel Shilovsky wrote:
> 
> > Do you need the #ifdef CONFIG_CIFS_SMB2 here considering that there
> > are
> > no such macro definitions around the declarations around
> > delayed_work
> > reconnect in struct TCP_Server_Info. It would also lead to unused
> > variable in case the kernel is compiled without CONFIG_CIFS_SMB2.
> 
> Thank you for reviewing this.
> 
> I posted the next version of the patch yesterday ([PATCH 4/5] CIFS:
> Fix a possible memory corruption during reconnect) that addresses
> this
> problem by moving fields inside TCP_Server_Info to CONFIG_CIFS_SMB2.
> 

Thanks. I'll review the patch separately.


> > > +             /* No need to send echo on newly established
> > > connections */
> > > +             mod_delayed_work(cifsiod_wq, &server->reconnect,
> > > 0);
> > > +             return rc;
> > 
> > Shouldn't this be queue_work?
> 
> It is made it this way because of the following: if someone submitted
> the work with a delay previously (now the code doesn't do it but it
> is
> possible in the future), we want to overwrite this delay to 0 since
> we
> need an immediate session/tcon reconnect here.
> 

OK. It should work without any problem but I would have stuck to the
expected functions and use mod_delayed_work() only when new code which
queues the work separately has been added. It is strange to see only
mod_delayed_work() when the work isn't queued anywhere else. Apart from
that small concern, I am fine with it.

Sachin Prabhu