All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
@ 2010-02-17  8:01 Rishikesh
  2010-02-17 17:10 ` Garrett Cooper
  0 siblings, 1 reply; 19+ messages in thread
From: Rishikesh @ 2010-02-17  8:01 UTC (permalink / raw)
  To: Shi Weihua; +Cc: LTP

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

-- 
Thanks&  Regards
Rishi Kesh K Rajak
IBM LTC, Bangalore
LTP Maintainer
Please join IRC: #ltp @ freenode.net


------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
  2010-02-17  8:01 [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1 Rishikesh
@ 2010-02-17 17:10 ` Garrett Cooper
  2010-02-18  4:25   ` Rishikesh K Rajak
  0 siblings, 1 reply; 19+ messages in thread
From: Garrett Cooper @ 2010-02-17 17:10 UTC (permalink / raw)
  To: Rishikesh; +Cc: LTP

On Wed, Feb 17, 2010 at 12:01 AM, Rishikesh <risrajak@linux.vnet.ibm.com> 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.

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).
Thanks,
-Garrett

------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
  2010-02-17 17:10 ` Garrett Cooper
@ 2010-02-18  4:25   ` Rishikesh K Rajak
  2010-02-22  7:15     ` Shi Weihua
  0 siblings, 1 reply; 19+ messages in thread
From: Rishikesh K Rajak @ 2010-02-18  4:25 UTC (permalink / raw)
  To: Garrett Cooper, Shi Weihua; +Cc: LTP

On Wed, Feb 17, 2010 at 09:10:48AM -0800, Garrett Cooper wrote:
> On Wed, Feb 17, 2010 at 12:01 AM, Rishikesh <risrajak@linux.vnet.ibm.com> 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@ .

Thanks for your support.
- Rishi

> Thanks,
> -Garrett

------------------------------------------------------------------------------
Download Intel&reg; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs 
proactively, and fine-tune applications for parallel performance. 
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
  2010-02-18  4:25   ` Rishikesh K Rajak
@ 2010-02-22  7:15     ` Shi Weihua
  2010-02-22  7:44       ` Garrett Cooper
  0 siblings, 1 reply; 19+ messages in thread
From: Shi Weihua @ 2010-02-22  7:15 UTC (permalink / raw)
  To: Rishikesh K Rajak; +Cc: LTP

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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
---
--- 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@cn.fujitsu.com
  *
  * 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) {
+					tst_resm(TFAIL|TTERRNO, "Got unxpected error");
 				} else {
-					tst_resm(TPASS, "Got expected EPERM "
-						 "error");
+					tst_resm(TPASS|TTERRNO, "Got expected error");
 				}
 			}
 

Thank you.
- Shi Weihua

> 
> Thanks for your support.
> - Rishi
> 
>> Thanks,
>> -Garrett
> 
> 

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
  2010-02-22  7:15     ` Shi Weihua
@ 2010-02-22  7:44       ` Garrett Cooper
  2010-02-23  1:48         ` Shi Weihua
  0 siblings, 1 reply; 19+ messages in thread
From: Garrett Cooper @ 2010-02-22  7:44 UTC (permalink / raw)
  To: Shi Weihua; +Cc: LTP

Ok.. one last thing.

On Sun, Feb 21, 2010 at 11:15 PM, Shi Weihua <shiwh@cn.fujitsu.com> 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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
> ---
> --- 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@cn.fujitsu.com
>  *
>  * 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] ?

> +                                       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);

    I've been debating about doing this in a macro out of include/,
but I haven't gotten there yet...

Thanks,
-Garrett

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
  2010-02-22  7:44       ` Garrett Cooper
@ 2010-02-23  1:48         ` Shi Weihua
  2010-02-25  1:18           ` Garrett Cooper
  0 siblings, 1 reply; 19+ messages in thread
From: Shi Weihua @ 2010-02-23  1:48 UTC (permalink / raw)
  To: Garrett Cooper; +Cc: LTP

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@cn.fujitsu.com> 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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
>> ---
>> --- 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@cn.fujitsu.com
>>  *
>>  * 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@cn.fujitsu.com
  *
  * 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]);
 				}
 			}
 


> 
>     I've been debating about doing this in a macro out of include/,
> but I haven't gotten there yet...
> 
> Thanks,
> -Garrett
> 
> 

-- 
Shi Weihua

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
  2010-02-23  1:48         ` Shi Weihua
@ 2010-02-25  1:18           ` Garrett Cooper
  2010-03-04  8:38             ` Shi Weihua
  0 siblings, 1 reply; 19+ messages in thread
From: Garrett Cooper @ 2010-02-25  1:18 UTC (permalink / raw)
  To: Shi Weihua; +Cc: LTP

On Mon, Feb 22, 2010 at 5:48 PM, Shi Weihua <shiwh@cn.fujitsu.com> 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@cn.fujitsu.com> 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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
>>> ---
>>> --- 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@cn.fujitsu.com
>>>  *
>>>  * 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@cn.fujitsu.com
>  *
>  * 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.

Thanks,
-Garrett

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
  2010-02-25  1:18           ` Garrett Cooper
@ 2010-03-04  8:38             ` Shi Weihua
  2010-03-04 18:35                 ` Garrett Cooper
  2010-03-04 18:37                 ` Garrett Cooper
  0 siblings, 2 replies; 19+ messages in thread
From: Shi Weihua @ 2010-03-04  8:38 UTC (permalink / raw)
  To: Garrett Cooper; +Cc: LTP

at 2010-2-25 9:18, Garrett Cooper wrote:
> On Mon, Feb 22, 2010 at 5:48 PM, Shi Weihua <shiwh@cn.fujitsu.com> 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@cn.fujitsu.com> 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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
>>>> ---
>>>> --- 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@cn.fujitsu.com
>>>>  *
>>>>  * 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@cn.fujitsu.com
>>  *
>>  * 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.

> 
> Thanks,
> -Garrett
> 
> 

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
  2010-03-04  8:38             ` Shi Weihua
@ 2010-03-04 18:35                 ` Garrett Cooper
  2010-03-04 18:37                 ` Garrett Cooper
  1 sibling, 0 replies; 19+ messages in thread
From: Garrett Cooper @ 2010-03-04 18:35 UTC (permalink / raw)
  To: Shi Weihua; +Cc: Rishikesh K Rajak, LTP, linux-kernel, ebiederm

On Thu, Mar 4, 2010 at 12:38 AM, Shi Weihua <shiwh@cn.fujitsu.com> wrote:
> at 2010-2-25 9:18, Garrett Cooper wrote:
>> On Mon, Feb 22, 2010 at 5:48 PM, Shi Weihua <shiwh@cn.fujitsu.com> 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@cn.fujitsu.com> 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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
>>>>> ---
>>>>> --- 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@cn.fujitsu.com
>>>>>  *
>>>>>  * 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@cn.fujitsu.com
>>>  *
>>>  * 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.

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.

Thanks,
-Garrett

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
@ 2010-03-04 18:35                 ` Garrett Cooper
  0 siblings, 0 replies; 19+ messages in thread
From: Garrett Cooper @ 2010-03-04 18:35 UTC (permalink / raw)
  To: Shi Weihua; +Cc: LTP, linux-kernel, ebiederm

On Thu, Mar 4, 2010 at 12:38 AM, Shi Weihua <shiwh@cn.fujitsu.com> wrote:
> at 2010-2-25 9:18, Garrett Cooper wrote:
>> On Mon, Feb 22, 2010 at 5:48 PM, Shi Weihua <shiwh@cn.fujitsu.com> 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@cn.fujitsu.com> 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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
>>>>> ---
>>>>> --- 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@cn.fujitsu.com
>>>>>  *
>>>>>  * 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@cn.fujitsu.com
>>>  *
>>>  * 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.

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.

Thanks,
-Garrett

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
  2010-03-04  8:38             ` Shi Weihua
@ 2010-03-04 18:37                 ` Garrett Cooper
  2010-03-04 18:37                 ` Garrett Cooper
  1 sibling, 0 replies; 19+ messages in thread
From: Garrett Cooper @ 2010-03-04 18:37 UTC (permalink / raw)
  To: Shi Weihua; +Cc: Rishikesh K Rajak, LTP, linux-kernel, ebiederm

On Thu, Mar 4, 2010 at 12:38 AM, Shi Weihua <shiwh@cn.fujitsu.com> wrote:
> at 2010-2-25 9:18, Garrett Cooper wrote:
>> On Mon, Feb 22, 2010 at 5:48 PM, Shi Weihua <shiwh@cn.fujitsu.com> 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@cn.fujitsu.com> 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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
>>>>> ---
>>>>> --- 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@cn.fujitsu.com
>>>>>  *
>>>>>  * 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@cn.fujitsu.com
>>>  *
>>>  * 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.

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.

Thanks,
-Garrett

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
@ 2010-03-04 18:37                 ` Garrett Cooper
  0 siblings, 0 replies; 19+ messages in thread
From: Garrett Cooper @ 2010-03-04 18:37 UTC (permalink / raw)
  To: Shi Weihua; +Cc: LTP, linux-kernel, ebiederm

On Thu, Mar 4, 2010 at 12:38 AM, Shi Weihua <shiwh@cn.fujitsu.com> wrote:
> at 2010-2-25 9:18, Garrett Cooper wrote:
>> On Mon, Feb 22, 2010 at 5:48 PM, Shi Weihua <shiwh@cn.fujitsu.com> 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@cn.fujitsu.com> 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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
>>>>> ---
>>>>> --- 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@cn.fujitsu.com
>>>>>  *
>>>>>  * 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@cn.fujitsu.com
>>>  *
>>>  * 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.

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.

Thanks,
-Garrett

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
  2010-03-04 18:37                 ` Garrett Cooper
@ 2010-03-04 19:57                   ` Eric W. Biederman
  -1 siblings, 0 replies; 19+ messages in thread
From: Eric W. Biederman @ 2010-03-04 19:57 UTC (permalink / raw)
  To: Garrett Cooper; +Cc: Shi Weihua, Rishikesh K Rajak, LTP, linux-kernel

Garrett Cooper <yanegomi@gmail.com> writes:

> On Thu, Mar 4, 2010 at 12:38 AM, Shi Weihua <shiwh@cn.fujitsu.com> wrote:
>> at 2010-2-25 9:18, Garrett Cooper wrote:
>>> On Mon, Feb 22, 2010 at 5:48 PM, Shi Weihua <shiwh@cn.fujitsu.com> 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@cn.fujitsu.com> 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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
>>>>>> ---
>>>>>> --- 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@cn.fujitsu.com
>>>>>>  *
>>>>>>  * 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@cn.fujitsu.com
>>>>  *
>>>>  * 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
@ 2010-03-04 19:57                   ` Eric W. Biederman
  0 siblings, 0 replies; 19+ messages in thread
From: Eric W. Biederman @ 2010-03-04 19:57 UTC (permalink / raw)
  To: Garrett Cooper; +Cc: LTP, linux-kernel

Garrett Cooper <yanegomi@gmail.com> writes:

> On Thu, Mar 4, 2010 at 12:38 AM, Shi Weihua <shiwh@cn.fujitsu.com> wrote:
>> at 2010-2-25 9:18, Garrett Cooper wrote:
>>> On Mon, Feb 22, 2010 at 5:48 PM, Shi Weihua <shiwh@cn.fujitsu.com> 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@cn.fujitsu.com> 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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
>>>>>> ---
>>>>>> --- 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@cn.fujitsu.com
>>>>>>  *
>>>>>>  * 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@cn.fujitsu.com
>>>>  *
>>>>  * 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


------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
  2010-03-04 19:57                   ` Eric W. Biederman
@ 2010-03-05  7:20                     ` Garrett Cooper
  -1 siblings, 0 replies; 19+ messages in thread
From: Garrett Cooper @ 2010-03-05  7:20 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: Shi Weihua, Rishikesh K Rajak, LTP, linux-kernel

On Thu, Mar 4, 2010 at 11:57 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Garrett Cooper <yanegomi@gmail.com> writes:
>
>> On Thu, Mar 4, 2010 at 12:38 AM, Shi Weihua <shiwh@cn.fujitsu.com> wrote:
>>> at 2010-2-25 9:18, Garrett Cooper wrote:
>>>> On Mon, Feb 22, 2010 at 5:48 PM, Shi Weihua <shiwh@cn.fujitsu.com> 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@cn.fujitsu.com> 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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
>>>>>>> ---
>>>>>>> --- 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@cn.fujitsu.com
>>>>>>>  *
>>>>>>>  * 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@cn.fujitsu.com
>>>>>  *
>>>>>  * 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;
> }

    Wow. Took a second for me to stare and it and see what you mean,
but yeah -- that is pretty dang awesome that it was always hardwired
to return 0.

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

    Well, if someone bumbles across this later, it will be a confusing
issue to work through. It's better to be documented instead of
undocumented. I'll file the bug upstream to document this, but it
would be nice to determine if there are any more immediate gaps which
need to be addressed in the changes.

>> 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.

    Hmmm.. ok. We just get 20 questions when something fails and it's
not documented why it should fail in a particular manner :).

> 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.

    Yeah, you're probably right (especially because Linux tends not to
focus on sysctl(3) like the BSDs do).

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

    Hmmm... ok. I assume that sysctl(2) is going completely out the
window in the future, in favor of what (just out of curiosity)? 100%
sysfs only tunables maybe?
Thanks,
-Garrett

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
@ 2010-03-05  7:20                     ` Garrett Cooper
  0 siblings, 0 replies; 19+ messages in thread
From: Garrett Cooper @ 2010-03-05  7:20 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: LTP, linux-kernel

On Thu, Mar 4, 2010 at 11:57 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Garrett Cooper <yanegomi@gmail.com> writes:
>
>> On Thu, Mar 4, 2010 at 12:38 AM, Shi Weihua <shiwh@cn.fujitsu.com> wrote:
>>> at 2010-2-25 9:18, Garrett Cooper wrote:
>>>> On Mon, Feb 22, 2010 at 5:48 PM, Shi Weihua <shiwh@cn.fujitsu.com> 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@cn.fujitsu.com> 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@linux.vnet.ibm.com> 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@cn.fujitsu.com>
>>>>>>> ---
>>>>>>> --- 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@cn.fujitsu.com
>>>>>>>  *
>>>>>>>  * 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@cn.fujitsu.com
>>>>>  *
>>>>>  * 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;
> }

    Wow. Took a second for me to stare and it and see what you mean,
but yeah -- that is pretty dang awesome that it was always hardwired
to return 0.

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

    Well, if someone bumbles across this later, it will be a confusing
issue to work through. It's better to be documented instead of
undocumented. I'll file the bug upstream to document this, but it
would be nice to determine if there are any more immediate gaps which
need to be addressed in the changes.

>> 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.

    Hmmm.. ok. We just get 20 questions when something fails and it's
not documented why it should fail in a particular manner :).

> 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.

    Yeah, you're probably right (especially because Linux tends not to
focus on sysctl(3) like the BSDs do).

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

    Hmmm... ok. I assume that sysctl(2) is going completely out the
window in the future, in favor of what (just out of curiosity)? 100%
sysfs only tunables maybe?
Thanks,
-Garrett

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
  2010-03-05  7:20                     ` Garrett Cooper
@ 2010-03-05  7:50                       ` Eric W. Biederman
  -1 siblings, 0 replies; 19+ messages in thread
From: Eric W. Biederman @ 2010-03-05  7:50 UTC (permalink / raw)
  To: Garrett Cooper; +Cc: Shi Weihua, Rishikesh K Rajak, LTP, linux-kernel

Garrett Cooper <yanegomi@gmail.com> writes:

> On Thu, Mar 4, 2010 at 11:57 AM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Garrett Cooper <yanegomi@gmail.com> writes:
>>>
>>> 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;
>> }
>
>     Wow. Took a second for me to stare and it and see what you mean,
> but yeah -- that is pretty dang awesome that it was always hardwired
> to return 0.

Sorry that wasn't clear without context.  What used to happen
where all of the callers of that function did:
if (test_perm(...))
   return -EPERM;

Instead of the much more conventional:

err = test_perm()
if (err)
   return err;

>> I admit that the manpage doesn't document EACCES but the manpage
>> has always said don't use sysctl(2) so...
>
>     Well, if someone bumbles across this later, it will be a confusing
> issue to work through. It's better to be documented instead of
> undocumented. I'll file the bug upstream to document this, but it
> would be nice to determine if there are any more immediate gaps which
> need to be addressed in the changes.

I think the linux test project may be nearly the only caller of sysctl(2)
at this point.  At least until recently there was one caller in arm
glibc.  But finding any program that uses sysctl(2) is nearly impossible.

>> 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.
>
>     Hmmm.. ok. We just get 20 questions when something fails and it's
> not documented why it should fail in a particular manner :).

>> 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.
>
>     Yeah, you're probably right (especially because Linux tends not to
> focus on sysctl(3) like the BSDs do).
>
>> At this point nothing should change again until such time as the code
>> is disabled/removed by default.
>
>     Hmmm... ok. I assume that sysctl(2) is going completely out the
> window in the future, in favor of what (just out of curiosity)? 100%
> sysfs only tunables maybe?

/proc/sys is going to stay.  Which is what people have actually used.
Even /sbin/sysctl has always used /proc/sys.  Nothing anyone actually
uses is going to go away.  Just the practically dead code that is the
syscall is slowly going away.  Since I have written the emulation layer
the need for it to disappear is less immediate than it once was, but I will
strongly discourage anyone from using it.

Eric

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
@ 2010-03-05  7:50                       ` Eric W. Biederman
  0 siblings, 0 replies; 19+ messages in thread
From: Eric W. Biederman @ 2010-03-05  7:50 UTC (permalink / raw)
  To: Garrett Cooper; +Cc: LTP, linux-kernel

Garrett Cooper <yanegomi@gmail.com> writes:

> On Thu, Mar 4, 2010 at 11:57 AM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Garrett Cooper <yanegomi@gmail.com> writes:
>>>
>>> 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;
>> }
>
>     Wow. Took a second for me to stare and it and see what you mean,
> but yeah -- that is pretty dang awesome that it was always hardwired
> to return 0.

Sorry that wasn't clear without context.  What used to happen
where all of the callers of that function did:
if (test_perm(...))
   return -EPERM;

Instead of the much more conventional:

err = test_perm()
if (err)
   return err;

>> I admit that the manpage doesn't document EACCES but the manpage
>> has always said don't use sysctl(2) so...
>
>     Well, if someone bumbles across this later, it will be a confusing
> issue to work through. It's better to be documented instead of
> undocumented. I'll file the bug upstream to document this, but it
> would be nice to determine if there are any more immediate gaps which
> need to be addressed in the changes.

I think the linux test project may be nearly the only caller of sysctl(2)
at this point.  At least until recently there was one caller in arm
glibc.  But finding any program that uses sysctl(2) is nearly impossible.

>> 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.
>
>     Hmmm.. ok. We just get 20 questions when something fails and it's
> not documented why it should fail in a particular manner :).

>> 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.
>
>     Yeah, you're probably right (especially because Linux tends not to
> focus on sysctl(3) like the BSDs do).
>
>> At this point nothing should change again until such time as the code
>> is disabled/removed by default.
>
>     Hmmm... ok. I assume that sysctl(2) is going completely out the
> window in the future, in favor of what (just out of curiosity)? 100%
> sysfs only tunables maybe?

/proc/sys is going to stay.  Which is what people have actually used.
Even /sbin/sysctl has always used /proc/sys.  Nothing anyone actually
uses is going to go away.  Just the practically dead code that is the
syscall is slowly going away.  Since I have written the emulation layer
the need for it to disappear is less immediate than it once was, but I will
strongly discourage anyone from using it.

Eric

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1
@ 2010-02-01 11:25 Shi Weihua
  0 siblings, 0 replies; 19+ messages in thread
From: Shi Weihua @ 2010-02-01 11:25 UTC (permalink / raw)
  To: ltp-list

The syscall of sysctl has been changed function call process on 2.6.33-rc1.
So we should change some code about sysctl.

Before 2.6.33-rc1:
do_sysctl
	->parse_table
		->sysctl_perm		(will return EPERM sometime)
		->do_sysctl_strategy
			->sysctl_perm	(will return EPERM sometime)
...

After 2.6.33-rc1:
do_sysctl
	->binary_sysctl
		check enable to access or not	(will return EACCES some time)
...

I fixed sysctl03, other cases of sysctl are OK.
This patch works on 2.6.31.5-127.fc12.x86_64, 2.6.32, 2.6.33-rc1, 
and 2.6.33-rc6.

Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com>
---
--- testcases/kernel/syscalls/sysctl/sysctl03.c.orig	2010-02-02 18:15:56.000000000 +0800
+++ testcases/kernel/syscalls/sysctl/sysctl03.c	2010-02-02 18:49:40.000000000 +0800
@@ -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@cn.fujitsu.com
  *
  * 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,13 @@ int main(int ac, char **av)
 		} else {
 			TEST_ERROR_LOG(TEST_ERRNO);
 
-			if (TEST_ERRNO != EPERM) {
+			if (TEST_ERRNO != exp_eno) {
 				tst_resm(TFAIL,
-					 "Expected EPERM (%d), got %d: %s",
-					 EPERM, TEST_ERRNO,
+					 "Expected %s (%d), got %d: %s",
+					 strerror(exp_eno), exp_eno, TEST_ERRNO,
 					 strerror(TEST_ERRNO));
 			} else {
-				tst_resm(TPASS, "Got expected EPERM error");
+				tst_resm(TPASS, "Got expected %s error", strerror(exp_eno));
 			}
 		}
 
@@ -147,12 +159,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_eno) {
+					tst_resm(TFAIL, "Expected %s, got "
+						 "%d", strerror(exp_eno), TEST_ERRNO);
 				} else {
-					tst_resm(TPASS, "Got expected EPERM "
-						 "error");
+					tst_resm(TPASS, "Got expected %s "
+						 "error", strerror(exp_eno));
 				}
 			}
 

------------------------------------------------------------------------------
The Planet: dedicated and managed hosting, cloud storage, colocation
Stay online with enterprise data centers and the best network in the business
Choose flexible plans and management services without long-term contracts
Personal 24x7 support from experience hosting pros just a phone call away.
http://p.sf.net/sfu/theplanet-com
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2010-03-05  7:50 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-02-17  8:01 [LTP] [PATCH] sysctl03: sysctl returns EACCES after 2.6.33-rc1 Rishikesh
2010-02-17 17:10 ` Garrett Cooper
2010-02-18  4:25   ` Rishikesh K Rajak
2010-02-22  7:15     ` Shi Weihua
2010-02-22  7:44       ` Garrett Cooper
2010-02-23  1:48         ` Shi Weihua
2010-02-25  1:18           ` Garrett Cooper
2010-03-04  8:38             ` Shi Weihua
2010-03-04 18:35               ` Garrett Cooper
2010-03-04 18:35                 ` Garrett Cooper
2010-03-04 18:37               ` Garrett Cooper
2010-03-04 18:37                 ` Garrett Cooper
2010-03-04 19:57                 ` Eric W. Biederman
2010-03-04 19:57                   ` Eric W. Biederman
2010-03-05  7:20                   ` Garrett Cooper
2010-03-05  7:20                     ` Garrett Cooper
2010-03-05  7:50                     ` Eric W. Biederman
2010-03-05  7:50                       ` Eric W. Biederman
  -- strict thread matches above, loose matches on Subject: below --
2010-02-01 11:25 Shi Weihua

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.