5:34 p.m.
Hi,
I am having a problem with ACLs in NFS Ganesha v2.6.3. Based on what I believe to be the
cause of the problem, I think the problem will exist in the tip too. I am using a
proprietary fsal.
The symptom of the problem is simple. If I create a file and quickly read the ACLs,
nothing is returned:
# echo test > test.txt
# nfs4_getfacl test.txt
#
If I restart ganesha, I see the correct ACLs:
# nfs4_getfacl test.txt
A::OWNER@:rwatTnNcCy
A::GROUP@:rtncy
A::EVERYONE@:rtncy
Also, if I create a file, and wait over 60 seconds, then I can read the correct ACLs.
This is all working fine in v2.5.1.
I think I see the problem in the code. In my case, the nfs client first sends a GETATTR
request with the ACL bit set in the request mask. But then it sends another one without
the ACL bit set. During this second request, when ACLs are not requested, the entry in
mdcache is updated in mdcache_refresh_attrs.
In 2.5.1, mdcache_refresh_attrs, after the subfsal is called to get the attributes, it
does:
if (entry->attrs.acl != NULL) {
/* We used to have an ACL... */
if (need_acl) {
/* We requested update of an existing ACL, release the
* old one.
*/
nfs4_acl_release_entry(entry->attrs.acl);
} else {
/* The ACL wasn't requested, move it into the
* new attributes so we will retain it and make
* it such that the entry attrs DO request the
* ACL.
*/
attrs.acl = entry->attrs.acl;
attrs.valid_mask |= ATTR_ACL;
entry->attrs.request_mask |= ATTR_ACL;
}
/* ACL was released or moved to new attributes. */
entry->attrs.acl = NULL;
}
Here, the second path is taken. Basically it moves the ACLs to attrs, but it sets the
entry's request mask bit. Soon after, fsal_copy_attrs is called which copies the ACLs
back to the entry. It does this because the entry's request mask ACL bit is set. If it
was not set, the ACLs would not be copied.
Unfortunately, this is not happening in 2.6.3:
In 2.6.3, this functionality was moved into mdc_update_attr_cache:
if (entry->attrs.acl != NULL) {
/* We used to have an ACL... */
if (attrs->acl != NULL) {
/* We got an ACL from the sub FSAL whether we asked for
* it or not, given that we had an ACL before, and we
* got a new one, update the ACL, so release the old
* one.
*/
nfs4_acl_release_entry(entry->attrs.acl);
} else {
/* A new ACL wasn't provided, so move the old one
* into the new attributes so it will be preserved
* by the fsal_copy_attrs.
*/
attrs->acl = entry->attrs.acl;
attrs->valid_mask |= ATTR_ACL;
}
/* NOTE: Because we already had an ACL,
* entry->attrs.request_mask MUST have the ATTR_ACL bit set.
* This will assure that fsal_copy_attrs below will copy the
* selected ACL (old or new) into entry->attrs.
*/
/* ACL was released or moved to new attributes. */
entry->attrs.acl = NULL;
} else if (attrs->acl != NULL) {
/* We didn't have an ACL before, but we got a new one. We may
* not have asked for it, but receive it anyway.
*/
entry->attrs.request_mask |= ATTR_ACL;
}
We are taking the path where it says "A new ACL wasn't provided". It is
moving the existing ACL into attrs, which we would like to be copied later by
fsal_copy_attrs, just like in 2.5.1. But note in 2.6, we are never setting the ACL bit in
the entry's request mask, only in attrs. Therefore, fsal_copy_attrs will not copy the
ACL back to the entry, and the ACL will be lost forever. Even worse, is the trusted ACL
flag in mde_flags is set, so now the caller thinks there are no ACLs at all!
The logic looks the same in the current 2.8 code to me.
Please let me know if my analysis is correct or not, and if so, if there is any possible
workaround.
Bill
9:05 a.m.
I think this should already work. As the comment there says, if the
entry has an ACL, it's request_mask should have ATTR_ACL set. Looking
through the code, it looks like the acl ptr and ATTR_ACL are always set
together.
Daniel
On 3/1/19 6:34 PM, katcherw(a)gmail.com wrote:
Hi,
I am having a problem with ACLs in NFS Ganesha v2.6.3. Based on what I believe to be the
cause of the problem, I think the problem will exist in the tip too. I am using a
proprietary fsal.
The symptom of the problem is simple. If I create a file and quickly read the ACLs,
nothing is returned:
# echo test > test.txt
# nfs4_getfacl test.txt
#
If I restart ganesha, I see the correct ACLs:
# nfs4_getfacl test.txt
A::OWNER@:rwatTnNcCy
A::GROUP@:rtncy
A::EVERYONE@:rtncy
Also, if I create a file, and wait over 60 seconds, then I can read the correct ACLs.
This is all working fine in v2.5.1.
I think I see the problem in the code. In my case, the nfs client first sends a GETATTR
request with the ACL bit set in the request mask. But then it sends another one without
the ACL bit set. During this second request, when ACLs are not requested, the entry in
mdcache is updated in mdcache_refresh_attrs.
In 2.5.1, mdcache_refresh_attrs, after the subfsal is called to get the attributes, it
does:
if (entry->attrs.acl != NULL) {
/* We used to have an ACL... */
if (need_acl) {
/* We requested update of an existing ACL, release the
* old one.
*/
nfs4_acl_release_entry(entry->attrs.acl);
} else {
/* The ACL wasn't requested, move it into the
* new attributes so we will retain it and make
* it such that the entry attrs DO request the
* ACL.
*/
attrs.acl = entry->attrs.acl;
attrs.valid_mask |= ATTR_ACL;
entry->attrs.request_mask |= ATTR_ACL;
}
/* ACL was released or moved to new attributes. */
entry->attrs.acl = NULL;
}
Here, the second path is taken. Basically it moves the ACLs to attrs, but it sets the
entry's request mask bit. Soon after, fsal_copy_attrs is called which copies the ACLs
back to the entry. It does this because the entry's request mask ACL bit is set. If it
was not set, the ACLs would not be copied.
Unfortunately, this is not happening in 2.6.3:
In 2.6.3, this functionality was moved into mdc_update_attr_cache:
if (entry->attrs.acl != NULL) {
/* We used to have an ACL... */
if (attrs->acl != NULL) {
/* We got an ACL from the sub FSAL whether we asked for
* it or not, given that we had an ACL before, and we
* got a new one, update the ACL, so release the old
* one.
*/
nfs4_acl_release_entry(entry->attrs.acl);
} else {
/* A new ACL wasn't provided, so move the old one
* into the new attributes so it will be preserved
* by the fsal_copy_attrs.
*/
attrs->acl = entry->attrs.acl;
attrs->valid_mask |= ATTR_ACL;
}
/* NOTE: Because we already had an ACL,
* entry->attrs.request_mask MUST have the ATTR_ACL bit set.
* This will assure that fsal_copy_attrs below will copy the
* selected ACL (old or new) into entry->attrs.
*/
/* ACL was released or moved to new attributes. */
entry->attrs.acl = NULL;
} else if (attrs->acl != NULL) {
/* We didn't have an ACL before, but we got a new one. We may
* not have asked for it, but receive it anyway.
*/
entry->attrs.request_mask |= ATTR_ACL;
}
We are taking the path where it says "A new ACL wasn't provided". It is
moving the existing ACL into attrs, which we would like to be copied later by
fsal_copy_attrs, just like in 2.5.1. But note in 2.6, we are never setting the ACL bit in
the entry's request mask, only in attrs. Therefore, fsal_copy_attrs will not copy the
ACL back to the entry, and the ACL will be lost forever. Even worse, is the trusted ACL
flag in mde_flags is set, so now the caller thinks there are no ACLs at all!
The logic looks the same in the current 2.8 code to me.
Please let me know if my analysis is correct or not, and if so, if there is any possible
workaround.
Bill
_______________________________________________
Devel mailing list -- devel(a)lists.nfs-ganesha.org
To unsubscribe send an email to devel-leave(a)lists.nfs-ganesha.org
12:52 p.m.
Daniel,
I don't think this assumption is always correct. In mdcache_refresh_attrs:
/* We will want all the requested attributes in the entry */
entry->attrs.request_mask = attrs.request_mask;
In my situation, this is turning off the ATTR_ACL bit in the entry's request mask.
Bill
1:25 p.m.
On 3/4/19 1:52 PM, katcherw(a)gmail.com wrote:
Daniel,
I don't think this assumption is always correct. In mdcache_refresh_attrs:
/* We will want all the requested attributes in the entry */
entry->attrs.request_mask = attrs.request_mask;
In my situation, this is turning off the ATTR_ACL bit in the entry's request mask.
Okay, I see. Something like this:
https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/446980
Daniel
3:02 p.m.
On 3/4/19 1:52 PM, katcherw(a)gmail.com wrote:
> Daniel,
>
> I don't think this assumption is always correct. In mdcache_refresh_attrs:
>
> /* We will want all the requested attributes in the entry */
> entry->attrs.request_mask = attrs.request_mask;
>
> In my situation, this is turning off the ATTR_ACL bit in the entry's request
mask.
>
Okay, I see. Something like this:
https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/446980
I've been looking over the code. That simple fix might be enough, but I'm not
sure.
I think our attribute tracking has gotten overly complex and some things are
disconnected.
One thought I have is for the time stamp of an attribute to actually be supplied by the
FSAL, at least conceptually (i.e. we could easily set it in mdcache_refresh_attrs just
after calling FSAL getattrs). But make the time stamps be part of struct attrlist.
Then each attribute (or group of attributes) has a bit in valid_mask, and a time stamp.
When the cached attributes are updated, the bit in the valid mask, the attribute(s), and
the time stamp are all copied together.
I wonder if we really need separate trust flags in mde_flags?
And we should make it such that request_mask is ONLY used to indicate to getattrs which
attributes are of interest (and of course mdcache may pass a different request_mask to the
underlying FSAL than it was passed).
In the current code, I see that fs_locations_time isn't ever checked, and there is no
separate sec_label_time...
Frank
7:49 a.m.
On 3/4/19 4:02 PM, Frank Filz wrote:
> On 3/4/19 1:52 PM, katcherw(a)gmail.com wrote:
>> Daniel,
>>
>> I don't think this assumption is always correct. In mdcache_refresh_attrs:
>>
>> /* We will want all the requested attributes in the entry */
>> entry->attrs.request_mask = attrs.request_mask;
>>
>> In my situation, this is turning off the ATTR_ACL bit in the entry's request
mask.
>>
>
> Okay, I see. Something like this:
>
>
> https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/446980
I've been looking over the code. That simple fix might be enough, but I'm not
sure.
I think our attribute tracking has gotten overly complex and some things are
disconnected.
One thought I have is for the time stamp of an attribute to actually be supplied by the
FSAL, at least conceptually (i.e. we could easily set it in mdcache_refresh_attrs just
after calling FSAL getattrs). But make the time stamps be part of struct attrlist.
Then each attribute (or group of attributes) has a bit in valid_mask, and a time stamp.
When the cached attributes are updated, the bit in the valid mask, the attribute(s), and
the time stamp are all copied together.
I wonder if we really need separate trust flags in mde_flags?
And we should make it such that request_mask is ONLY used to indicate to getattrs which
attributes are of interest (and of course mdcache may pass a different request_mask to the
underlying FSAL than it was passed).
In the current code, I see that fs_locations_time isn't ever checked, and there is no
separate sec_label_time...
Frank
I'm not adverse to changing how we handle attribute validity. However,
I think a timestamp-per-attribute would more than double the size of the
attrlist, and so I think something else is necessary. It would also
lead to a *ton* of timestamp comparisons for any getattr operation,
which would add to latency.
Daniel
9:10 a.m.
On 3/4/19 4:02 PM, Frank Filz wrote:
>> On 3/4/19 1:52 PM, katcherw(a)gmail.com wrote:
>>> Daniel,
>>>
>>> I don't think this assumption is always correct. In
mdcache_refresh_attrs:
>>>
>>> /* We will want all the requested attributes in the entry */
>>> entry->attrs.request_mask = attrs.request_mask;
>>>
>>> In my situation, this is turning off the ATTR_ACL bit in the entry's
request
mask.
>>>
>>
>> Okay, I see. Something like this:
>>
>>
>> https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/446980
>
> I've been looking over the code. That simple fix might be enough, but I'm
not
sure.
>
> I think our attribute tracking has gotten overly complex and some things are
disconnected.
>
> One thought I have is for the time stamp of an attribute to actually be supplied
by the FSAL, at least conceptually (i.e. we could easily set it in
mdcache_refresh_attrs just after calling FSAL getattrs). But make the time
stamps be part of struct attrlist.
>
> Then each attribute (or group of attributes) has a bit in valid_mask, and a time
stamp. When the cached attributes are updated, the bit in the valid mask, the
attribute(s), and the time stamp are all copied together.
>
> I wonder if we really need separate trust flags in mde_flags?
>
> And we should make it such that request_mask is ONLY used to indicate to
getattrs which attributes are of interest (and of course mdcache may pass a
different request_mask to the underlying FSAL than it was passed).
>
> In the current code, I see that fs_locations_time isn't ever checked, and there
is no separate sec_label_time...
>
> Frank
>
I'm not adverse to changing how we handle attribute validity. However, I think a
timestamp-per-attribute would more than double the size of the attrlist, and so I
think something else is necessary. It would also lead to a *ton* of timestamp
comparisons for any getattr operation, which would add to latency.
That's why I said "or group of attributes". In fact, other than adding a
time stamp for security label, I'm not proposing adding any more time stamps (though I
don't know if there might be a case for dividing the POSIX attributes out that are
updated by things like layout commit, the question there would be do clients ever request
a subset of attributes that are easily maintained by Ganesha without having to go to the
file system. Outside of that, all the traditional POSIX attributes need only be covered by
a single time stamp. Hmm, another place that might call for more granularity would be
really using STATX to request subsets of the POSIX attributes. These ideas would need to
be explored in more detail.
Frank
3:35 p.m.
Yes, I think that will work. I compiled in the patch and it so far looks like it solved my
problem. Thanks!
Bill
10:53 a.m.
There are unlikely to be any more 2.6 releases.
Daniel
On 3/6/19 10:04 AM, katcherw(a)gmail.com wrote:
Is there any chance of this being backported to the 2.6 branch?
Bill
_______________________________________________
Devel mailing list -- devel(a)lists.nfs-ganesha.org
To unsubscribe send an email to devel-leave(a)lists.nfs-ganesha.org
2163
days inactive
2168
days old
9 comments
3 participants
participants (3)
-
Daniel Gryniewicz -
Frank Filz -
katcherw＠gmail.com