ср, 29 авг. 2018 г. в 16:13, ronnie sahlberg <ronniesahlberg@xxxxxxxxx>:
>
> On Thu, Aug 30, 2018 at 9:01 AM, Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> > ср, 29 авг. 2018 г. в 15:34, Pavel Shilovsky <piastryyy@xxxxxxxxx>:
> >>
> >> вт, 28 авг. 2018 г. в 19:19, Ronnie Sahlberg <lsahlber@xxxxxxxxxx>:
> >> >
> >> > For compounded PDUs we whould only wake the waiting thread for the
> >> > very last PDU of the compound.
> >> > We do this so that we are guaranteed that the demultiplex_thread will
> >> > not process or access any of those MIDs any more once the send/recv
> >> > thread starts processing.
> >> >
> >> > Else there is a race where at the end of the send/recv processing we
> >> > will try to delete all the mids of the compound. If the multiplex
> >> > thread still has other mids to process at this point for this compound
> >> > this can lead to an oops.
> >> >
> >> > Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> >> > ---
> >> > fs/cifs/transport.c | 19 ++++++++++++++++++-
> >> > 1 file changed, 18 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> >> > index 9e3a1e87ae1c..49a800ce75d6 100644
> >> > --- a/fs/cifs/transport.c
> >> > +++ b/fs/cifs/transport.c
> >> > @@ -773,6 +773,11 @@ cifs_setup_request(struct cifs_ses *ses, struct smb_rqst *rqst)
> >> > return mid;
> >> > }
> >> >
> >> > +static void
> >> > +cifs_noop_callback(struct mid_q_entry *mid)
> >> > +{
> >> > +}
> >> > +
> >> > int
> >> > compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> >> > const int flags, const int num_rqst, struct smb_rqst *rqst,
> >> > @@ -826,9 +831,15 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> >> > return PTR_ERR(midQ[i]);
> >> > }
> >> >
> >> > + /*
> >> > + * We don't invoke the callback compounds unless it is the last
> >> > + * request.
> >> > + */
> >> > midQ[i]->mid_state = MID_REQUEST_SUBMITTED;
> >> > + if (ses->server->ops->next_header &&
> >> > + ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base))
> >> > + midQ[i]->callback = cifs_noop_callback;
> >>
> >> Wouldn't it be easier to just check for i < num_rqst - 1 ? Having
> >> ses->server->ops->next_header(rqst[i].rq_iov[0].iov_base) return false
> >> should be a bug here. SMB1 code doesn't support compounding at all, so
> >> num_rqst is always 1.
> >
> > Also I thought that we may use a similar change you did in WIP patch -
> > just set mids[i]->multiRsp and mids[i]->multiEnd properly for
> > compounded requests. I mean the following:
> >
> > if (num_req > 1) {
> > mids[i]->multiRsp = 1;
> > if (i == num_req - 1) {
> > mids[i]->multiEnd = 1;
> > }
> > }
> >
> > This will work for both compound and non-compound requests and utilize
> > the existing multiRsp/multiEnd fields. Thoughts?
>
> I don't think we can set those flags already in compound_send_recv
> since that will affect other codepaths during send and receive, at
> least handle_mid()
> possibly other places too.
>
> But your other suggestion to just look at num_rqst instead of checking
> for and calling ->next_header makes sense. I will do that and re-send.
>
Actually, it does seem that handle_mid() is the only place that
behaves differently depending on those flags. We could add another
mid->compound flag to check in handle_mid(). But anyway, that will
probably complicate things more because for SMB1 multiRsp/multiEnd
flags serve slightly another purpose. Agree, let's do indices check
instead.
--
Best regards,
Pavel Shilovsky
