Improve handling of retransmissions

It looks like the previous change was somewhat incomplete:

- I made a small boolean mistake in OPEN_CONFIRM, using && instead of
||. This causes retransmissions to misbehave.
- I overlooked the fact that OPEN_DOWNGRADE can also perform stricter
checks on the incoming request.

While there, add a utility function for doing these kinds of
comparisons. This makes the code a bit more readable.

Author: EdSchouten

pkg/filesystem/virtual/nfsv4/base_program.go

@@ -949,13 +949,7 @@ func (s *compoundState) opClose(args *nfsv4.Close4args) nfsv4.Close4res {
 	transaction, lastResponse, st := oos.startTransaction(p, args.Seqid, &ll, unconfirmedOpenOwnerPolicyDeny)
 	if st != nfsv4.NFS4_OK {
 		if r, ok := lastResponse.(nfsv4.Close4res); ok {
-			// Only return a cached response if the request
-			// contains the same state ID as provided
-			// originally.
-			//
-			// More details: RFC 7530, section 9.1.9, bullet
-			// point 3.
-			if okResponse, ok := lastResponse.(*nfsv4.Close4res_NFS4_OK); !ok || (okResponse.OpenStateid.Other == args.OpenStateid.Other && okResponse.OpenStateid.Seqid == nextSeqID(args.OpenStateid.Seqid)) {
+			if okResponse, ok := lastResponse.(*nfsv4.Close4res_NFS4_OK); !ok || isNextStateID(&okResponse.OpenStateid, &args.OpenStateid) {
 				return r
 			}
 		}
@@ -1168,13 +1162,7 @@ func (s *compoundState) opLock(args *nfsv4.Lock4args) nfsv4.Lock4res {
 		transaction, lastResponse, st := los.startTransaction(p, owner.LockSeqid, false)
 		if st != nfsv4.NFS4_OK {
 			if r, ok := lastResponse.(nfsv4.Lock4res); ok {
-				// Only return a cached response if the
-				// request contains the same state ID as
-				// provided originally.
-				//
-				// More details: RFC 7530, section
-				// 9.1.9, bullet point 3.
-				if okResponse, ok := lastResponse.(*nfsv4.Lock4res_NFS4_OK); !ok || (okResponse.Resok4.LockStateid.Other == owner.LockStateid.Other && okResponse.Resok4.LockStateid.Seqid != nextSeqID(owner.LockStateid.Seqid)) {
+				if okResponse, ok := lastResponse.(*nfsv4.Lock4res_NFS4_OK); !ok || isNextStateID(&okResponse.Resok4.LockStateid, &owner.LockStateid) {
 					return r
 				}
 			}
@@ -1385,13 +1373,7 @@ func (s *compoundState) opLocku(args *nfsv4.Locku4args) nfsv4.Locku4res {
 	transaction, lastResponse, st := los.startTransaction(p, args.Seqid, false)
 	if st != nfsv4.NFS4_OK {
 		if r, ok := lastResponse.(nfsv4.Locku4res); ok {
-			// Only return a cached response if the request
-			// contains the same state ID as provided
-			// originally.
-			//
-			// More details: RFC 7530, section 9.1.9, bullet
-			// point 3.
-			if okResponse, ok := lastResponse.(*nfsv4.Locku4res_NFS4_OK); !ok || (okResponse.LockStateid.Other == args.LockStateid.Other && okResponse.LockStateid.Seqid != nextSeqID(args.LockStateid.Seqid)) {
+			if okResponse, ok := lastResponse.(*nfsv4.Locku4res_NFS4_OK); !ok || isNextStateID(&okResponse.LockStateid, &args.LockStateid) {
 				return r
 			}
 		}
@@ -1725,13 +1707,7 @@ func (s *compoundState) opOpenConfirm(args *nfsv4.OpenConfirm4args) nfsv4.OpenCo
 	transaction, lastResponse, st := oos.startTransaction(p, args.Seqid, &ll, unconfirmedOpenOwnerPolicyAllow)
 	if st != nfsv4.NFS4_OK {
 		if r, ok := lastResponse.(nfsv4.OpenConfirm4res); ok {
-			// Only return a cached response if the request
-			// contains the same state ID as provided
-			// originally.
-			//
-			// More details: RFC 7530, section 9.1.9, bullet
-			// point 3.
-			if okResponse, ok := lastResponse.(*nfsv4.OpenConfirm4res_NFS4_OK); !ok && (okResponse.Resok4.OpenStateid.Other == args.OpenStateid.Other && okResponse.Resok4.OpenStateid.Seqid == nextSeqID(args.OpenStateid.Seqid)) {
+			if okResponse, ok := lastResponse.(*nfsv4.OpenConfirm4res_NFS4_OK); !ok || isNextStateID(&okResponse.Resok4.OpenStateid, &args.OpenStateid) {
 				return r
 			}
 		}
@@ -1780,7 +1756,9 @@ func (s *compoundState) opOpenDowngrade(args *nfsv4.OpenDowngrade4args) nfsv4.Op
 	transaction, lastResponse, st := oos.startTransaction(p, args.Seqid, &ll, unconfirmedOpenOwnerPolicyDeny)
 	if st != nfsv4.NFS4_OK {
 		if r, ok := lastResponse.(nfsv4.OpenDowngrade4res); ok {
-			return r
+			if okResponse, ok := lastResponse.(*nfsv4.OpenDowngrade4res_NFS4_OK); !ok || isNextStateID(&okResponse.Resok4.OpenStateid, &args.OpenStateid) {
+				return r
+			}
 		}
 		return &nfsv4.OpenDowngrade4res_default{Status: st}
 	}
@@ -1893,8 +1871,11 @@ func (s *compoundState) opReaddir(ctx context.Context, args *nfsv4.Readdir4args)
 	}
 
 	// Validate the cookie verifier.
+	// TODO: The macOS NFSv4 client may sometimes not set the cookie
+	// verifier properly, so we allow it to be zero. Remove this
+	// logic in due time. Issue: rdar://91034875
 	p := s.program
-	if args.Cookie != 0 && args.Cookieverf != p.rebootVerifier {
+	if args.Cookie != 0 && args.Cookieverf != p.rebootVerifier && (args.Cookieverf != nfsv4.Verifier4{}) {
 		return &nfsv4.Readdir4res_default{Status: nfsv4.NFS4ERR_NOT_SAME}
 	}
 
@@ -3036,6 +3017,20 @@ func nextSeqID(seqID nfsv4.Seqid4) nfsv4.Seqid4 {
 	return seqID + 1
 }
 
+// isNextStateID returns true if state ID a is the successor of state ID b.
+//
+// At a minimum, the standard states that when returning a cached
+// response to operations such as CLOSE, LOCK, LOCKU, OPEN_CONFIRM, and
+// OPEN_DOWNGRADE, it is sufficient to compare the original operation
+// type and the operation's sequence ID. This function can be used to
+// increase strictness by ensuring that the state IDs in the request
+// also match the originally provided values.
+//
+// More details: RFC 7530, section 9.1.9, bullet point 3.
+func isNextStateID(a, b *nfsv4.Stateid4) bool {
+	return a.Other == b.Other && a.Seqid == nextSeqID(b.Seqid)
+}
+
 // toShareMask converts NFSv4 share_access values that are part of OPEN
 // and OPEN_DOWNGRADE requests to our equivalent ShareMask values.
 func shareAccessToShareMask(in uint32) (virtual.ShareMask, nfsv4.Nfsstat4) {

pkg/filesystem/virtual/nfsv4/base_program_test.go

@@ -2764,7 +2764,122 @@ func TestBaseProgramCompound_OP_OPENATTR(t *testing.T) {
 	})
 }
 
-// TODO: OPEN_CONFIRM
+func TestBaseProgramCompound_OP_OPEN_CONFIRM(t *testing.T) {
+	ctrl, ctx := gomock.WithContext(context.Background(), t)
+
+	rootDirectory := mock.NewMockVirtualDirectory(ctrl)
+	rootDirectory.EXPECT().VirtualGetAttributes(gomock.Any(), virtual.AttributesMaskFileHandle, gomock.Any()).
+		Do(func(ctx context.Context, requested virtual.AttributesMask, attributes *virtual.Attributes) {
+			attributes.SetFileHandle([]byte{0x2e, 0x8d, 0x48, 0x03, 0xc4, 0xc3, 0x2d, 0x6c})
+		})
+	handleResolver := mock.NewMockHandleResolver(ctrl)
+	randomNumberGenerator := mock.NewMockSingleThreadedGenerator(ctrl)
+	rebootVerifier := nfsv4_xdr.Verifier4{0x42, 0xa8, 0x3f, 0xd1, 0xde, 0x65, 0x74, 0x2a}
+	stateIDOtherPrefix := [...]byte{0xfa, 0xc3, 0xf7, 0x18}
+	clock := mock.NewMockClock(ctrl)
+	program := nfsv4.NewBaseProgram(rootDirectory, handleResolver.Call, randomNumberGenerator, rebootVerifier, stateIDOtherPrefix, clock, 2*time.Minute, time.Minute)
+
+	clock.EXPECT().Now().Return(time.Unix(1000, 0))
+	clock.EXPECT().Now().Return(time.Unix(1001, 0))
+	setClientIDForTesting(ctx, t, randomNumberGenerator, program, 0x2e5550c498b2b463)
+
+	t.Run("RetransmissionSuccess", func(t *testing.T) {
+		// It should be valid to send OPEN_CONFIRM repeatedly in
+		// case of connection drops.
+		leaf := mock.NewMockVirtualLeaf(ctrl)
+		clock.EXPECT().Now().Return(time.Unix(1002, 0))
+		clock.EXPECT().Now().Return(time.Unix(1003, 0))
+		openUnconfirmedFileForTesting(
+			ctx,
+			t,
+			randomNumberGenerator,
+			program,
+			rootDirectory,
+			leaf,
+			nfsv4_xdr.NfsFh4{0xff, 0x27, 0xc7, 0x8f, 0xd5, 0x6a, 0xfb, 0xee},
+			/* shortClientID = */ 0x2e5550c498b2b463,
+			/* seqID = */ 1205,
+			/* stateIDOther = */ [...]byte{
+				0xfa, 0xc3, 0xf7, 0x18,
+				0x80, 0x57, 0x5b, 0x95,
+				0x08, 0x16, 0x41, 0x0a,
+			})
+
+		for i := int64(0); i < 10; i++ {
+			clock.EXPECT().Now().Return(time.Unix(1004+i*2, 0))
+			clock.EXPECT().Now().Return(time.Unix(1005+i*2, 0))
+			openConfirmForTesting(
+				ctx,
+				t,
+				randomNumberGenerator,
+				program,
+				nfsv4_xdr.NfsFh4{0xff, 0x27, 0xc7, 0x8f, 0xd5, 0x6a, 0xfb, 0xee},
+				/* seqID = */ 1206,
+				/* stateIDOther = */ [...]byte{
+					0xfa, 0xc3, 0xf7, 0x18,
+					0x80, 0x57, 0x5b, 0x95,
+					0x08, 0x16, 0x41, 0x0a,
+				})
+		}
+	})
+
+	t.Run("RetransmissionWithMismatchingStateID", func(t *testing.T) {
+		// At a minimum, the standard states that when returning
+		// a cached response it is sufficient to compare the
+		// original operation type and sequence ID. Let's be a
+		// bit more strict and actually check whether the
+		// provided state ID matches the one that was provided
+		// as part of the original request.
+		//
+		// More details: RFC 7530, section 9.1.9, bullet point 3.
+		clock.EXPECT().Now().Return(time.Unix(1024, 0))
+		clock.EXPECT().Now().Return(time.Unix(1025, 0))
+
+		res, err := program.NfsV4Nfsproc4Compound(ctx, &nfsv4_xdr.Compound4args{
+			Tag: "close",
+			Argarray: []nfsv4_xdr.NfsArgop4{
+				&nfsv4_xdr.NfsArgop4_OP_PUTFH{
+					Opputfh: nfsv4_xdr.Putfh4args{
+						Object: nfsv4_xdr.NfsFh4{0xff, 0x27, 0xc7, 0x8f, 0xd5, 0x6a, 0xfb, 0xee},
+					},
+				},
+				&nfsv4_xdr.NfsArgop4_OP_OPEN_CONFIRM{
+					OpopenConfirm: nfsv4_xdr.OpenConfirm4args{
+						Seqid: 1206,
+						OpenStateid: nfsv4_xdr.Stateid4{
+							Seqid: 3,
+							Other: [...]byte{
+								0xfa, 0xc3, 0xf7, 0x18,
+								0x80, 0x57, 0x5b, 0x95,
+								0x08, 0x16, 0x41, 0x0a,
+							},
+						},
+					},
+				},
+			},
+		})
+		require.NoError(t, err)
+		require.Equal(t, &nfsv4_xdr.Compound4res{
+			Tag: "close",
+			Resarray: []nfsv4_xdr.NfsResop4{
+				&nfsv4_xdr.NfsResop4_OP_PUTFH{
+					Opputfh: nfsv4_xdr.Putfh4res{
+						Status: nfsv4_xdr.NFS4_OK,
+					},
+				},
+				&nfsv4_xdr.NfsResop4_OP_OPEN_CONFIRM{
+					OpopenConfirm: &nfsv4_xdr.OpenConfirm4res_default{
+						Status: nfsv4_xdr.NFS4ERR_BAD_SEQID,
+					},
+				},
+			},
+			Status: nfsv4_xdr.NFS4ERR_BAD_SEQID,
+		}, res)
+	})
+
+	// TODO: Any more cases we want to test?
+}
+
 // TODO: OPEN_DOWNGRADE
 // TODO: PUTFH
 // TODO: PUTPUBFH