Skip to content

Commit

Permalink
Improve handling of retransmissions
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@EdSchouten
EdSchouten committed Jul 13, 2023
1 parent b1a8462 commit fc589ed
Show file tree
Hide file tree
Showing 2 changed files with 141 additions and 31 deletions.
55 changes: 25 additions & 30 deletions pkg/filesystem/virtual/nfsv4/base_program.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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}
}
Expand Down Expand Up @@ -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}
}

Expand Down Expand Up @@ -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 *nfsv4.Stateid4, 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) {
Expand Down
117 changes: 116 additions & 1 deletion pkg/filesystem/virtual/nfsv4/base_program_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit fc589ed

Please sign in to comment.