[nfs-discuss] Code review for 6775211: mirror mounts use the zcred; should use caller's c
Nicolas Williams wrote:
> On Wed, Feb 11, 2009 at 11:09:33AM -0600, Tom Haynes wrote:
>
>> Nicolas Williams wrote:
>>
>>> 782 +crset_zone_privall(mcred);
>>> 783 +if (crsetpriv(mcred, PRIV_FILE_DAC_WRITE,
>>> PRIV_FILE_DAC_READ,
>>> 784 +PRIV_FILE_DAC_SEARCH, PRIV_FILE_DAC_EXECUTE,
>>> PRIV_SYS_MOUNT,
>>> 785 +PRIV_FILE_OWNER, NULL)) {
>>>
>>> Why crsetpriv() when the crset_zone_privall() at 782 should be getting
>>> you all those privs anyways?
>>>
>>> (And if the zone doesn't have some of those, then maybe the user should
>>> get what they deserver :)
>>>
>> Okay, I've tested a stock system, a system with the above piece of
>> code, and a system without the above piece of code. The stock system
>> shows the error and both of the other two test systems do not show the
>> error.
>>
>> That means this code is indeed not needed.
>>
>> I'm going to keep the old version at the above webrev. I've generated
>> a new one at:
>>
>> http://cr.opensolaris.org/~tdh/krbvariant
>>
>
> Thanks.
>
> You seem to have indented 693-694 by accident.
>
From pulling out the EBUSY bug.
I *hate* how webrev doesn't identify this.
You looked at the straight diffs?
I've refreshed the krbvariant webrev.
> Looks good!
>
> Please RTI and push ASAP :)
>
>
I'll need a second reviewer to do that.
> And the EBUSY bug too!
>
> Nico
>
[nfs-discuss] Code review for 6775211: mirror mounts use the zcred; should use caller's c
On Wed, Feb 11, 2009 at 11:09:33AM -0600, Tom Haynes wrote:
> Nicolas Williams wrote:
> > 782 +crset_zone_privall(mcred);
> > 783 +if (crsetpriv(mcred, PRIV_FILE_DAC_WRITE,
> > PRIV_FILE_DAC_READ,
> > 784 +PRIV_FILE_DAC_SEARCH, PRIV_FILE_DAC_EXECUTE,
> > PRIV_SYS_MOUNT,
> > 785 +PRIV_FILE_OWNER, NULL)) {
> >
> >Why crsetpriv() when the crset_zone_privall() at 782 should be getting
> >you all those privs anyways?
> >
> >(And if the zone doesn't have some of those, then maybe the user should
> >get what they deserver :)
>
> Okay, I've tested a stock system, a system with the above piece of
> code, and a system without the above piece of code. The stock system
> shows the error and both of the other two test systems do not show the
> error.
>
> That means this code is indeed not needed.
>
> I'm going to keep the old version at the above webrev. I've generated
> a new one at:
>
> http://cr.opensolaris.org/~tdh/krbvariant
Thanks.
You seem to have indented 693-694 by accident.
Looks good!
Please RTI and push ASAP :)
And the EBUSY bug too!
Nico
--
[nfs-discuss] Code review for 6775211: mirror mounts use the zcred; should use caller's c
Nicolas Williams wrote:
> On Fri, Jan 30, 2009 at 03:34:19PM -0800, Tom Haynes wrote:
>
>> http://bugs.opensolaris.org/view_bug.do?bug_id=6775211
>>
>> For more than you want to know about this bug, see
>> http://blogs.sun.com/tdh/entry/be_careful_of_assumptions
>>
>> The basic issue is if a NFSv4 client has user credentials, but no
>> machine credentials, then it will not be able to access kerberized
>> shares via mirrormounts.
>>
>> The reason is that we used the zcred to force the mounts to work for
>> users. The correct solution is to add privileges to the user
>> credential.
>>
>> I'd like some code reviews, say by the middle of next week.
>>
>> http://cr.opensolaris.org/~tdh/krbonly/
>>
>
> 782 +crset_zone_privall(mcred);
> 783 +if (crsetpriv(mcred, PRIV_FILE_DAC_WRITE,
> PRIV_FILE_DAC_READ,
> 784 +PRIV_FILE_DAC_SEARCH, PRIV_FILE_DAC_EXECUTE,
> PRIV_SYS_MOUNT,
> 785 +PRIV_FILE_OWNER, NULL)) {
>
> Why crsetpriv() when the crset_zone_privall() at 782 should be getting
> you all those privs anyways?
>
> (And if the zone doesn't have some of those, then maybe the user should
> get what they deserver :)
>
Okay, I've tested a stock system, a system with the above piece of code,
and a system
without the above piece of code. The stock system shows the error and
both of the
other two test systems do not show the error.
That means this code is indeed not needed.
I'm going to keep the old version at the above webrev. I've generated a
new one at:
http://cr.opensolaris.org/~tdh/krbvariant
> Also, why not include the fix for the spurious EBUSYs that I saw
> earlier?
>
> Thanks,
>
> Nico
>
[nfs-discuss] Code review for 6775211: mirror mounts use the zcred; should use caller's c
Nicolas Williams wrote:
> On Fri, Jan 30, 2009 at 03:34:19PM -0800, Tom Haynes wrote:
>
>> http://bugs.opensolaris.org/view_bug.do?bug_id=6775211
>>
>> For more than you want to know about this bug, see
>> http://blogs.sun.com/tdh/entry/be_careful_of_assumptions
>>
>> The basic issue is if a NFSv4 client has user credentials, but no
>> machine credentials, then it will not be able to access kerberized
>> shares via mirrormounts.
>>
>> The reason is that we used the zcred to force the mounts to work for
>> users. The correct solution is to add privileges to the user
>> credential.
>>
>> I'd like some code reviews, say by the middle of next week.
>>
>> http://cr.opensolaris.org/~tdh/krbonly/
>>
>
> 782 +crset_zone_privall(mcred);
> 783 +if (crsetpriv(mcred, PRIV_FILE_DAC_WRITE,
> PRIV_FILE_DAC_READ,
> 784 +PRIV_FILE_DAC_SEARCH, PRIV_FILE_DAC_EXECUTE,
> PRIV_SYS_MOUNT,
> 785 +PRIV_FILE_OWNER, NULL)) {
>
> Why crsetpriv() when the crset_zone_privall() at 782 should be getting
> you all those privs anyways?
>
>
It isn't setting them.
Or at least it wasn't in Dec when I coded/tested up this function.
I can clone a new tree and take this stuff out to see if that is still
the case, but I seem to recall that the
privs were not there.
> (And if the zone doesn't have some of those, then maybe the user should
> get what they deserver :)
>
>
> Also, why not include the fix for the spurious EBUSYs that I saw
> earlier?
>
> Thanks,
>
> Nico
>
Because that fix didn't pass unit testing - the mirror mounts did not
automatically unmount.
[nfs-discuss] Code review for 6775211: mirror mounts use the zcred; should use caller's c
On Fri, Jan 30, 2009 at 03:34:19PM -0800, Tom Haynes wrote:
> http://bugs.opensolaris.org/view_bug.do?bug_id=6775211
>
> For more than you want to know about this bug, see
> http://blogs.sun.com/tdh/entry/be_careful_of_assumptions
>
> The basic issue is if a NFSv4 client has user credentials, but no
> machine credentials, then it will not be able to access kerberized
> shares via mirrormounts.
>
> The reason is that we used the zcred to force the mounts to work for
> users. The correct solution is to add privileges to the user
> credential.
>
> I'd like some code reviews, say by the middle of next week.
>
> http://cr.opensolaris.org/~tdh/krbonly/
782 +crset_zone_privall(mcred);
783 +if (crsetpriv(mcred, PRIV_FILE_DAC_WRITE, PRIV_FILE_DAC_READ,
784 +PRIV_FILE_DAC_SEARCH, PRIV_FILE_DAC_EXECUTE,
PRIV_SYS_MOUNT,
785 +PRIV_FILE_OWNER, NULL)) {
Why crsetpriv() when the crset_zone_privall() at 782 should be getting
you all those privs anyways?
(And if the zone doesn't have some of those, then maybe the user should
get what they deserver :)
Also, why not include the fix for the spurious EBUSYs that I saw
earlier?
Thanks,
Nico
--
[nfs-discuss] Code review for 6775211: mirror mounts use the zcred; should use caller's c
http://bugs.opensolaris.org/view_bug.do?bug_id=6775211 For more than you want to know about this bug, see http://blogs.sun.com/tdh/entry/be_careful_of_assumptions The basic issue is if a NFSv4 client has user credentials, but no machine credentials, then it will not be able to access kerberized shares via mirrormounts. The reason is that we used the zcred to force the mounts to work for users. The correct solution is to add privileges to the user credential. I'd like some code reviews, say by the middle of next week. http://cr.opensolaris.org/~tdh/krbonly/ -- This message posted from opensolaris.org
