Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1

From: Eric W. Biederman
Date: Thu Mar 04 2010 - 14:57:43 EST

Next message: Rafael J. Wysocki: "Re: [linux-pm] Is it supposed to be ok to call del_gendisk while userspace is frozen?"
Previous message: john stultz: "Re: [PATCH 14/14] arm: Convert arm to use read_persistent_clock"
In reply to: Garrett Cooper: "Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1"
Next in thread: Garrett Cooper: "Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Garrett Cooper <yanegomi@xxxxxxxxx> writes:

> On Thu, Mar 4, 2010 at 12:38 AM, Shi Weihua <shiwh@xxxxxxxxxxxxxx> wrote:
>> at 2010-2-25 9:18, Garrett Cooper wrote:
>>> On Mon, Feb 22, 2010 at 5:48 PM, Shi Weihua <shiwh@xxxxxxxxxxxxxx> wrote:
>>>> at 2010-2-22 15:44, Garrett Cooper wrote:
>>>>> Ok.. one last thing.
>>>>>
>>>>> On Sun, Feb 21, 2010 at 11:15 PM, Shi Weihua <shiwh@xxxxxxxxxxxxxx> wrote:
>>>>>> at 2010-2-18 12:25, Rishikesh K Rajak wrote:
>>>>>>> On Wed, Feb 17, 2010 at 09:10:48AM -0800, Garrett Cooper wrote:
>>>>>>>> On Wed, Feb 17, 2010 at 12:01 AM, Rishikesh <risrajak@xxxxxxxxxxxxxxxxxx> wrote:
>>>>>>>>> Hi Shi,
>>>>>>>>>
>>>>>>>>> Thanks for your patch, still i am finding time to look into your patch.
>>>>>>>>> Will reply you soon.
>>>>>>>>>
>>>>>>>>> http://marc.info/?l=ltp-list&m=126502355614857&w=2
>>>>>>>>
>>>>>>>> I looked at the patch finally.
>>>>>>>
>>>>>>> Thanks garret.
>>>>>>>>
>>>>>>>> Please use TTERRNO instead of strerror(...) for the reported value.
>>>>>>>> There's no reason why you should use strerror(...) there as that's
>>>>>>>> already done with tst_res(3).
>>>>>>>
>>>>>>> Shi, can you please incorporate changes which garret has suggested and
>>>>>>> resend the patch again to ltp-list@ .
>>>>>>
>>>>>> ÂOk, i recreated the patch based on garret's suggestion.
>>>>>>
>>>>>> Signed-off-by: Shi Weihua <shiwh@xxxxxxxxxxxxxx>
>>>>>> ---
>>>>>> --- testcases/kernel/syscalls/sysctl/sysctl03.c.orig Â Â2010-02-22 14:38:26.000000000 -0500
>>>>>> +++ testcases/kernel/syscalls/sysctl/sysctl03.c 2010-02-22 15:08:03.000000000 -0500
>>>>>> @@ -22,15 +22,18 @@
>>>>>> Â* Â Â sysctl03.c
>>>>>> Â*
>>>>>> Â* DESCRIPTION
>>>>>> - * Â Â Testcase to check that sysctl(2) sets errno to EPERM correctly.
>>>>>> + * Â Â Testcase to check that sysctl(2) sets errno to EPERM or EACCES
>>>>>> + * Â Â correctly. But it will return EACCES on kernels that are 2.6.33-rc1
>>>>>> + * Â Â and higher.
>>>>>> Â*
>>>>>> Â* ALGORITHM
>>>>>> Â* Â Â a. Â Â ÂCall sysctl(2) as a root user, and attempt to write data
>>>>>> Â* Â Â Â Â Â Â to the kernel_table[]. Since the table does not have write
>>>>>> - * Â Â Â Â Â Â permissions even for the root, it should fail EPERM.
>>>>>> + * Â Â Â Â Â Â permissions even for the root, it should fail EPERM or EACCES.
>>>>>> Â* Â Â b. Â Â ÂCall sysctl(2) as a non-root user, and attempt to write data
>>>>>> Â* Â Â Â Â Â Â to the kernel_table[]. Since the table does not have write
>>>>>> - * Â Â Â Â Â Â permission for the regular user, it should fail with EPERM.
>>>>>> + * Â Â Â Â Â Â permission for the regular user, it should fail with EPERM
>>>>>> + * Â Â Â Â Â Â or EACCES.
>>>>>> Â*
>>>>>> Â* USAGE: Â<for command-line>
>>>>>> Â* Âsysctl03 [-c n] [-e] [-i n] [-I x] [-P x] [-t]
>>>>>> @@ -43,6 +46,7 @@
>>>>>> Â*
>>>>>> Â* HISTORY
>>>>>> Â* Â Â 07/2001 Ported by Wayne Boyer
>>>>>> + * Â Â 02/2010 Updated by shiwh@xxxxxxxxxxxxxx
>>>>>> Â*
>>>>>> Â* RESTRICTIONS
>>>>>> Â* Â Â Test must be run as root.
>>>>>> @@ -82,6 +86,7 @@ int main(int ac, char **av)
>>>>>> Â{
>>>>>> Â Â Â Âint lc;
>>>>>> Â Â Â Âchar *msg;
>>>>>> + Â Â Â int exp_eno;
>>>>>>
>>>>>> Â Â Â Âchar osname[OSNAMESZ];
>>>>>> Â Â Â Âint osnamelth, status;
>>>>>> @@ -96,6 +101,13 @@ int main(int ac, char **av)
>>>>>>
>>>>>> Â Â Â Âsetup();
>>>>>>
>>>>>> + Â Â Â if ((tst_kvercmp(2, 6, 32)) <= 0){
>>>>>> + Â Â Â Â Â Â Â exp_eno = EPERM;
>>>>>> + Â Â Â } else {
>>>>>> + Â Â Â Â Â Â Â exp_eno = EACCES;
>>>>>> + Â Â Â Â Â Â Â exp_enos[0] = EACCES;
>>>>>> + Â Â Â }
>>>>>> +
>>>>>> Â Â Â ÂTEST_EXP_ENOS(exp_enos);
>>>>>>
>>>>>> Â Â Â Â/* check looping state if -i option is given */
>>>>>> @@ -114,13 +126,10 @@ int main(int ac, char **av)
>>>>>> Â Â Â Â Â Â Â Â} else {
>>>>>> Â Â Â Â Â Â Â Â Â Â Â ÂTEST_ERROR_LOG(TEST_ERRNO);
>>>>>>
>>>>>> - Â Â Â Â Â Â Â Â Â Â Â if (TEST_ERRNO != EPERM) {
>>>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TFAIL,
>>>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â"Expected EPERM (%d), got %d: %s",
>>>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂEPERM, TEST_ERRNO,
>>>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstrerror(TEST_ERRNO));
>>>>>> + Â Â Â Â Â Â Â Â Â Â Â if (TEST_ERRNO != exp_eno) {
>>>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TFAIL|TTERRNO, "Got unxpected error");
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â} else {
>>>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TPASS, "Got expected EPERM error");
>>>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TPASS|TTERRNO, "Got expected error");
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â}
>>>>>> Â Â Â Â Â Â Â Â}
>>>>>>
>>>>>> @@ -147,12 +156,10 @@ int main(int ac, char **av)
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â} else {
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂTEST_ERROR_LOG(TEST_ERRNO);
>>>>>>
>>>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (TEST_ERRNO != EPERM) {
>>>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TFAIL, "Expected EPERM, got "
>>>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â"%d", TEST_ERRNO);
>>>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (TEST_ERRNO != exp_eno) {
>>>>>
>>>>> Why can't this be exp_enos[0] ?
>>>>
>>>> Using exp_enos[0] is ok. pls check the latest patch in this mail.
>>>>
>>>>>
>>>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TFAIL|TTERRNO, "Got unxpected error");
>>>>>
>>>>> Typo.
>>>>>
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â} else {
>>>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TPASS, "Got expected EPERM "
>>>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â"error");
>>>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TPASS|TTERRNO, "Got expected error");
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â}
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â}
>>>>>>
>>>>>>
>>>>>> Thank you.
>>>>>> - Shi Weihua
>>>>>
>>>>> Â Â It always helps to understand what's expected vs unexpected
>>>>> without having to look at the source code. Could you please revise the
>>>>> tst_resm format strings to be something like the following?
>>>>>
>>>>> tst_resm (TPASS | TERRNO, "Got expected error (errno = %d, ret_code =
>>>>> %d", exp_errno_blah, exp_ret_code_blah);
>>>>>
>>>>> OR:
>>>>>
>>>>> tst_resm (TPASS | TERRNO, "Got unexpected error (expected errno = %d,
>>>>> ret_code = %d - got errno = %d, ret_code = %d", exp_errno_blah,
>>>>> exp_ret_code_blah, errno_blah, ret_code_blah);
>>>>
>>>> ret_code has not been defined in sysctl03.c, i think it's unnecessarily to
>>>> import a new parameter. maybe, we can use your macro when it created. ;-)
>>>>
>>>> --- testcases/kernel/syscalls/sysctl/sysctl03.c.orig Â Â2010-02-22 14:38:26.000000000 -0500
>>>> +++ testcases/kernel/syscalls/sysctl/sysctl03.c 2010-02-23 09:35:20.000000000 -0500
>>>> @@ -22,15 +22,18 @@
>>>> Â* Â Â sysctl03.c
>>>> Â*
>>>> Â* DESCRIPTION
>>>> - * Â Â Testcase to check that sysctl(2) sets errno to EPERM correctly.
>>>> + * Â Â Testcase to check that sysctl(2) sets errno to EPERM or EACCES
>>>> + * Â Â correctly. But it will return EACCES on kernels that are 2.6.33-rc1
>>>> + * Â Â and higher.
>>>> Â*
>>>> Â* ALGORITHM
>>>> Â* Â Â a. Â Â ÂCall sysctl(2) as a root user, and attempt to write data
>>>> Â* Â Â Â Â Â Â to the kernel_table[]. Since the table does not have write
>>>> - * Â Â Â Â Â Â permissions even for the root, it should fail EPERM.
>>>> + * Â Â Â Â Â Â permissions even for the root, it should fail EPERM or EACCES.
>>>> Â* Â Â b. Â Â ÂCall sysctl(2) as a non-root user, and attempt to write data
>>>> Â* Â Â Â Â Â Â to the kernel_table[]. Since the table does not have write
>>>> - * Â Â Â Â Â Â permission for the regular user, it should fail with EPERM.
>>>> + * Â Â Â Â Â Â permission for the regular user, it should fail with EPERM
>>>> + * Â Â Â Â Â Â or EACCES.
>>>> Â*
>>>> Â* USAGE: Â<for command-line>
>>>> Â* Âsysctl03 [-c n] [-e] [-i n] [-I x] [-P x] [-t]
>>>> @@ -43,6 +46,7 @@
>>>> Â*
>>>> Â* HISTORY
>>>> Â* Â Â 07/2001 Ported by Wayne Boyer
>>>> + * Â Â 02/2010 Updated by shiwh@xxxxxxxxxxxxxx
>>>> Â*
>>>> Â* RESTRICTIONS
>>>> Â* Â Â Test must be run as root.
>>>> @@ -96,6 +100,12 @@ int main(int ac, char **av)
>>>>
>>>> Â Â Â Âsetup();
>>>>
>>>> + Â Â Â if ((tst_kvercmp(2, 6, 32)) <= 0){
>>>> + Â Â Â Â Â Â Â exp_enos[0] = EPERM;
>>>> + Â Â Â } else {
>>>> + Â Â Â Â Â Â Â exp_enos[0] = EACCES;
>>>> + Â Â Â }
>>>> +
>>>> Â Â Â ÂTEST_EXP_ENOS(exp_enos);
>>>>
>>>> Â Â Â Â/* check looping state if -i option is given */
>>>> @@ -114,13 +124,12 @@ int main(int ac, char **av)
>>>> Â Â Â Â Â Â Â Â} else {
>>>> Â Â Â Â Â Â Â Â Â Â Â ÂTEST_ERROR_LOG(TEST_ERRNO);
>>>>
>>>> - Â Â Â Â Â Â Â Â Â Â Â if (TEST_ERRNO != EPERM) {
>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TFAIL,
>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â"Expected EPERM (%d), got %d: %s",
>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂEPERM, TEST_ERRNO,
>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstrerror(TEST_ERRNO));
>>>> + Â Â Â Â Â Â Â Â Â Â Â if (TEST_ERRNO != exp_enos[0]) {
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TFAIL|TTERRNO, "Got unxpected error "
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "(expected error = %d)", exp_enos[0]);
>>>> Â Â Â Â Â Â Â Â Â Â Â Â} else {
>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TPASS, "Got expected EPERM error");
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TPASS|TTERRNO, "Got expected error "
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "(errno = %d)", exp_enos[0]);
>>>> Â Â Â Â Â Â Â Â Â Â Â Â}
>>>> Â Â Â Â Â Â Â Â}
>>>>
>>>> @@ -147,12 +156,12 @@ int main(int ac, char **av)
>>>> Â Â Â Â Â Â Â Â Â Â Â Â} else {
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂTEST_ERROR_LOG(TEST_ERRNO);
>>>>
>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (TEST_ERRNO != EPERM) {
>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TFAIL, "Expected EPERM, got "
>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â"%d", TEST_ERRNO);
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (TEST_ERRNO != exp_enos[0]) {
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TFAIL|TTERRNO, "Got unxpected error "
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "(expected error = %d)", exp_enos[0]);
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â} else {
>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TPASS, "Got expected EPERM "
>>>> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â"error");
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tst_resm(TPASS|TTERRNO, "Got expected error "
>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â"(errno = %d)", Âexp_enos[0]);
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â}
>>>> Â Â Â Â Â Â Â Â Â Â Â Â}
>>>
>>> Ok, before I go and commit this, there are two things I need to know
>>> (because trusty Google didn't turn up any results for this behavioral
>>> change):
>>>
>>> 1. The change is legitimate.
>>> 2. The documentation is up to date, or a bug has been filed for the
>>> documentation change.
>>>
>>> If you can provide these two things, I'll commit the change.
>>
>> At this time of day, I can not find any documentation about EPERM->EACCES.
>> But, i caught the kernel commit which caused ltp bug:
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=26a7034b40ba80f82f64fa251a2cbf49f9971c6a
>> Maybe it can help you to judge.
>
> Wow... that's a fair amount of code refactoring and additions to the syscall.
>
> Yes, all of the issues with opening a directory and reading/writing
> now apply to sysctl(2), especially if someone attempts to read from a
> write-only descriptor, or vice versa.

No mismatches of file descriptor modes and how the descriptor is
accessed can not occur. There is a file descriptor but the file
descriptor is completely internal to binary_sysctl(), and it is opened
with the mode of what we are trying to use. There are no user space
controllable parts there.

Looking through the old sysctl code it appears that it was a bug that
kept it from returning EACCES. The code has had this beautiful snippet
in it for ages:

static int test_perm(int mode, int op)
{
if (!current->euid)
mode >>= 6;
else if (in_egroup_p(0))
mode >>= 3;
if ((mode & op & 0007) == op)
return 0;
return -EACCES;
}

I admit that the manpage doesn't document EACCES but the manpage
has always said don't use sysctl(2) so...

> I hate to do this in such a reactive manner as this should have been
> done up front, but the 1) documentation and 2) test need to be
> updated. 1) is more key today because I'm not sure how much testing
> the developer did before he committed to kernel.org, but without
> updated documentation and requirements we can't write proper tests.

You may see a slightly different error code from sysctl(2) on failure
but otherwise sysctl(2) should be unchanged, and yes I did test it.
Of course I was not being picky about which error code I got on failure.

What exists today is simply a backwards compatibility wrapper of
sysctl(2) built on top of /proc/sys. sysctl(2) was a practically
unmaintained bit-rotting pile, that was never adequately maintained or
tested.

At this point nothing should change again until such time as the code
is disabled/removed by default.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Rafael J. Wysocki: "Re: [linux-pm] Is it supposed to be ok to call del_gendisk while userspace is frozen?"
Previous message: john stultz: "Re: [PATCH 14/14] arm: Convert arm to use read_persistent_clock"
In reply to: Garrett Cooper: "Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1"
Next in thread: Garrett Cooper: "Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]