Bug 8330 - NFSv4 ACL merging logic is broken
Summary: NFSv4 ACL merging logic is broken
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: File services (show other bugs)
Version: 3.6.0rc3
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
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
View All Add an attachment (proposed patch, testcase, etc.)

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!