Ship It!

There was one weird smbtorture test that this code was dealing with.  Let me also check on that and get back to you.

How about we remove the return(-1) but leave the rest of the if (...status != 0) block?

I believe the spec. still calls out some "invalid parameter" cases for which we might need to do an smb2sr_put_error call (at some point in the future).

e.g. no matching protocol, I think.  I know I've seen Windows return invalid protocol to negotiate, so I know it's possible.

I'd rather not have smb2_status ignored here.

Oh, sorry.  I advised you incorrectly.

If we want to handle (e.g.) NT_STATUS_INVALID_PARAMETER here we need to do the put error here AND return zero.
Perhaps also add a comment here that there are no protocol failures after this point.

The rc!=0 test is just for the (probably impossible) case of running out of message space.

OK, I think I'd recommend adding the put error call here, just before the send reply call:

    if (sr->smb2_status != 0)
        smb2sr_put_error(sr, sr->smb2_status);

Also, if failure in smb2_negotiate_common is not going to cause connection drop, it might be better to change the return to a ulong NT_STATUS and let callers do the smb2sr_put_error like most other places.

This is a good start, but I'd like to ask for some restructuring.
We've tried to always place the START probes after everythign has been decoded (unless that's impractical for some reason) so that the START probe can examine the fully decoded state.

See smb2_create for a complex example of the expected code structure.
In general, every SMB2 dispatch handler should be structured like this:

Variable inializations and allocations
Decode fixed parts.
Decode variable parts. (generally, decode errors are a protocol violation and should cause a disconnect, as "all bets are off". Generally disconnect happens by returning a non-zero "dispatch code" to the common dispatch function.)
Call the dtrace START probe
Do error checking on what we decoded (errors after the START probe should goto a label just before the DONE probe)
Execute the "action" for the request, usually via some sub-function (possibly shared with SMB1)
Call the dtrace DONE probe
Encode the results (either success or failure).  In general, encoding should not fail.
Common cleanup of locals (may be a label here for goto from decode section errors)
Return to common dispatch (where the reply is sent, either immediately or at the end of a compound).

Note that Negotiate is unusual in that it's not called by the common dispatch function.

As written smb2_negotiate_common was trying to be like a normal dispatch handler function,

and smb2_newrq_negotiate and smb1_negotiate_smb2 were intended to contain the actions that

would normally be expected in the caller of a normal dispatch handler function.
Sorry that wasn't more clear.  I thought about putting that in a big comment somewhere, but I'm not sure where it would go that people would actually see it.  Reach out to me (privately) if you'd like to talk about any of this.  Thanks! -GWR

See later comment about doing more of this in smb2_negotiate_common() if possible.
Also a nit: why insert this one assignment here vs after the assignment of smb2_cmd_code?

See my comment in smb2_negotiate_common, relevant to these additions.

This move is breaking some rules (OK, unwritten rules) about the dtrace probes.  See my longer comment about this in the general part of the review.

Would it be difficult to keep all the "build an SMB2 reply" work here in smb2_negotiate_common()?

That was the intent of this funciton.

Thanks for simplifying this change.  I have just a few bits of feedback remaining.

I think we want the final (overwrite=true) call to smb2_encode_header() after smb2_status is set and after any calls to smb2sr_put_error(sr, status);

(Right before smb2_send_reply)
At least that's how it happens elsewhere.

Same for the later call at line 317

Did you forget to limit the version_cnt value that's passed to smb_mbc_decodef?

If you change this to

sr->smb2_hdr_flags |= SMB2_FLAGS_SERVER_TO_REDIR;

you could get rid of saved_hdr_flags.

Same comment as on the earlier smb2_encode_header call. (line 165)

One other thing I just noticed.  We really should be checking SMB2_FLAGS_SERVER_TO_REDIR here too (like smb2_dispatch does).  This is also a good place to do the "reserve header space" too (also like dispatch)

+   if ((sr->smb2_hdr_flags & SMB2_FLAGS_SERVER_TO_REDIR) != 0)
+       return (-1);
+   sr->smb2_hdr_flags |= SMB2_FLAGS_SERVER_TO_REDIR;
+
+   /* Reserve space for the reply header. */
+   (void) smb2_encode_header(sr, B_FALSE);

Thanks for your patience with all my nits etc.