Bug 8330 - NFSv4 ACL merging logic is broken
NFSv4 ACL merging logic is broken
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
3.6.0rc3
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-27 12:42 UTC by Christian Ambach
Modified: 2012-05-23 20:03 UTC (History)
2 users (show)

See Also:


Attachments
proposed patch (as in master) (1.30 KB, patch)
2011-07-27 14:45 UTC, Christian Ambach
obnox: review+
Details
Patch simplifying the check (1020 bytes, patch)
2011-07-28 07:57 UTC, Michael Adam
ambi: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Ambach 2011-07-27 12:42:13 UTC
The NFSv4 ACL merging logic that can be enabled with nfs4:acedup=merge incorrectly merges ACEs which have different flags like CI/OI/I

e.g.
ACL:BUILTIN\Users:ALLOWED/0x0/FULL
ACL:BUILTIN\Users:ALLOWED/I/READ

gets merged to
ACL:BUILTIN\Users:ALLOWED/I/FULL

This behaviour is wrong, ACEs with different flags cannot be merged.

Patch to come
Comment 1 Christian Ambach 2011-07-27 14:45:22 UTC
Created attachment 6723 [details]
proposed patch (as in master)
Comment 2 Michael Adam 2011-07-28 07:53:35 UTC
Comment on attachment 6723 [details]
proposed patch (as in master)

Yes, I think we should take this patch for 3.6.X.

But take a look at the subsequent patch I am going to attach
Comment 3 Michael Adam 2011-07-28 07:57:02 UTC
Created attachment 6724 [details]
Patch simplifying the check

I think when we pass (ace->aceFlags == aceNewFlags)
then the next condition is automatic:

(ace->aceFlags&SMB_ACE4_IDENTIFIER_GROUP == aceNew->aceFlags&SMB_ACE4_IDENTIFIER_GROUP)

So this patch removes this second check.
(Not yet pushed to master, ambi, please feel free to push.)
Comment 4 Michael Adam 2011-07-28 11:23:49 UTC
The simplification patch has been pushed to master.
It applies to v3-6-test unmodified (after applying the first patch).

Ambi:
* please review.
* Assign to Karolin if positive review is granted.

Karolin:
* No need to force this into 3.6.0 (if it is too late).
  But we should add it to the next release possible...

Cheers - Michael
Comment 5 Christian Ambach 2011-08-01 15:44:09 UTC
Comment on attachment 6724 [details]
Patch simplifying the check

yap, patch is good (i should have seen this obvious optimization by myself)
Comment 6 Christian Ambach 2011-08-01 15:45:05 UTC
Karolin,

this is for 3.6.x
Comment 7 Karolin Seeger 2011-08-09 11:18:46 UTC
Pushed to v3-6-test.
Will be included in 3.6.0.
Closing out bug report.

Thanks!