From: Stanislav K. <sta...@or...> - 2013-08-16 07:11:18
|
One of parameters to setgroups() syscall is (gid_t *) pointer. If TST_USE_COMPAT16_VSYSCALL is defined a pointer to GID_T is passed instead (and sizeof(GID_T) < sizeof(gid_t)). It's not safe and can result in unaligned access (and SIGBUS) on several platforms. Signed-off-by: Stanislav Kholmanskikh <sta...@or...> --- testcases/kernel/syscalls/setgroups/compat_16.h | 22 +++++++++++++++++++- testcases/kernel/syscalls/setgroups/setgroups04.c | 6 ++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/testcases/kernel/syscalls/setgroups/compat_16.h b/testcases/kernel/syscalls/setgroups/compat_16.h index 0de4e78..35723d6 100644 --- a/testcases/kernel/syscalls/setgroups/compat_16.h +++ b/testcases/kernel/syscalls/setgroups/compat_16.h @@ -32,9 +32,27 @@ extern void cleanup(void); #ifdef TST_USE_COMPAT16_SYSCALL long -SETGROUPS(size_t gidsetsize, GID_T *list) +SETGROUPS(size_t gidsetsize, GID_T *list16) { - return ltp_syscall(__NR_setgroups, gidsetsize, list); + int r; + int i; + + gid_t *list32; + + list32 = calloc(gidsetsize, sizeof(gid_t)); + if (list32 == NULL) + tst_brkm(TBROK | TERRNO, NULL, + "calloc failed to allocate %zu bytes at %s:%d", + gidsetsize * sizeof(gid_t), + __FILE__, __LINE__); + + for (i = 0; i < gidsetsize; i++) + list32[i] = list16[i]; + + r = ltp_syscall(__NR_setgroups, gidsetsize, list32); + + free(list32); + return r; } int diff --git a/testcases/kernel/syscalls/setgroups/setgroups04.c b/testcases/kernel/syscalls/setgroups/setgroups04.c index 5932b4e..42ddda2 100644 --- a/testcases/kernel/syscalls/setgroups/setgroups04.c +++ b/testcases/kernel/syscalls/setgroups/setgroups04.c @@ -111,7 +111,11 @@ int main(int ac, char **av) * verify that it fails with -1 return value and * sets appropriate errno. */ - TEST(SETGROUPS(gidsetsize, sbrk(0))); +#ifdef TST_USE_COMPAT16_SYSCALL + TEST(ltp_syscall(__NR_setgroups, gidsetsize, sbrk(0))); +#else + TEST(setgroups(gidsetsize, sbrk(0))); +#endif if (TEST_RETURN != -1) { tst_resm(TFAIL, "setgroups() returned %ld, " -- 1.7.1 |
From: <ch...@su...> - 2013-08-22 13:13:23
|
> One of parameters to setgroups() syscall is (gid_t *) pointer. > If TST_USE_COMPAT16_VSYSCALL is defined a pointer to GID_T is passed > instead (and sizeof(GID_T) < sizeof(gid_t)). It's not safe and > can result in unaligned access (and SIGBUS) on several platforms. > > Signed-off-by: Stanislav Kholmanskikh <sta...@or...> > --- > testcases/kernel/syscalls/setgroups/compat_16.h | 22 +++++++++++++++++++- > testcases/kernel/syscalls/setgroups/setgroups04.c | 6 ++++- > 2 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/testcases/kernel/syscalls/setgroups/compat_16.h b/testcases/kernel/syscalls/setgroups/compat_16.h > index 0de4e78..35723d6 100644 > --- a/testcases/kernel/syscalls/setgroups/compat_16.h > +++ b/testcases/kernel/syscalls/setgroups/compat_16.h > @@ -32,9 +32,27 @@ extern void cleanup(void); > #ifdef TST_USE_COMPAT16_SYSCALL > > long > -SETGROUPS(size_t gidsetsize, GID_T *list) > +SETGROUPS(size_t gidsetsize, GID_T *list16) > { > - return ltp_syscall(__NR_setgroups, gidsetsize, list); > + int r; > + int i; > + > + gid_t *list32; > + > + list32 = calloc(gidsetsize, sizeof(gid_t)); > + if (list32 == NULL) > + tst_brkm(TBROK | TERRNO, NULL, > + "calloc failed to allocate %zu bytes at %s:%d", > + gidsetsize * sizeof(gid_t), > + __FILE__, __LINE__); > + > + for (i = 0; i < gidsetsize; i++) > + list32[i] = list16[i]; > + > + r = ltp_syscall(__NR_setgroups, gidsetsize, list32); > + > + free(list32); > + return r; > } This looks like the __NR_setgroups is not the compact16 one we want. Look at the getgroups16 in kernel/uid16.c it calls groups16_from_user() and that does: for (i = 0; i < group_info->ngroups; i++) { if (get_user(group, grouplist+i)) return -EFAULT; The grouplist is of old_gid_t __user *grouplist type so it's unsigned short, so it works with array of unsigned short which is aligned to unsigned shorts due to definiton of the GID_T in the test sources, there is no way this would trigger unaligned access. And actually the GETGROUPS() seems to be coded to pass list32 and converts it to list16 which looks just wrong to me as it's inside of TST_USE_COMPAT16_SYSCALL. It just looks to me like we have wrong syscall number to begin with. -- Cyril Hrubis ch...@su... |
From: Stanislav K. <sta...@or...> - 2013-08-23 11:33:26
|
On 08/22/2013 05:13 PM, ch...@su... wrote: >> One of parameters to setgroups() syscall is (gid_t *) pointer. >> If TST_USE_COMPAT16_VSYSCALL is defined a pointer to GID_T is passed >> instead (and sizeof(GID_T) < sizeof(gid_t)). It's not safe and >> can result in unaligned access (and SIGBUS) on several platforms. >> >> Signed-off-by: Stanislav Kholmanskikh <sta...@or...> >> --- >> testcases/kernel/syscalls/setgroups/compat_16.h | 22 +++++++++++++++++++- >> testcases/kernel/syscalls/setgroups/setgroups04.c | 6 ++++- >> 2 files changed, 25 insertions(+), 3 deletions(-) >> >> diff --git a/testcases/kernel/syscalls/setgroups/compat_16.h b/testcases/kernel/syscalls/setgroups/compat_16.h >> index 0de4e78..35723d6 100644 >> --- a/testcases/kernel/syscalls/setgroups/compat_16.h >> +++ b/testcases/kernel/syscalls/setgroups/compat_16.h >> @@ -32,9 +32,27 @@ extern void cleanup(void); >> #ifdef TST_USE_COMPAT16_SYSCALL >> >> long >> -SETGROUPS(size_t gidsetsize, GID_T *list) >> +SETGROUPS(size_t gidsetsize, GID_T *list16) >> { >> - return ltp_syscall(__NR_setgroups, gidsetsize, list); >> + int r; >> + int i; >> + >> + gid_t *list32; >> + >> + list32 = calloc(gidsetsize, sizeof(gid_t)); >> + if (list32 == NULL) >> + tst_brkm(TBROK | TERRNO, NULL, >> + "calloc failed to allocate %zu bytes at %s:%d", >> + gidsetsize * sizeof(gid_t), >> + __FILE__, __LINE__); >> + >> + for (i = 0; i < gidsetsize; i++) >> + list32[i] = list16[i]; >> + >> + r = ltp_syscall(__NR_setgroups, gidsetsize, list32); >> + >> + free(list32); >> + return r; >> } > This looks like the __NR_setgroups is not the compact16 one we want. > > Look at the getgroups16 in kernel/uid16.c it calls groups16_from_user() > and that does: > > > for (i = 0; i < group_info->ngroups; i++) { > if (get_user(group, grouplist+i)) > return -EFAULT; > > > The grouplist is of old_gid_t __user *grouplist type so it's unsigned short, so > it works with array of unsigned short which is aligned to unsigned shorts due > to definiton of the GID_T in the test sources, there is no way this > would trigger unaligned access. > > And actually the GETGROUPS() seems to be coded to pass list32 and converts it > to list16 which looks just wrong to me as it's inside of > TST_USE_COMPAT16_SYSCALL. It just looks to me like we have wrong syscall > number to begin with. > Hi, Cyril. Thank you for this point. At first sight it seems that this testcase is very specific for i386 (maybe for other architectures also). In i386 I found two variants of setgroups syscall (from arch/x86/syscalls/syscall_32.tbl): 81 i386 setgroups sys_setgroups16 206 i386 setgroups32 sys_setgroups I guess in i386 glibc handler for setgroups() internally uses _setgroups32_ syscall. In x86_64 there is only one variant (from arch/x86/syscalls/syscall_64.tbl): 116 common setgroups sys_setgroups which points to 32-bit version. But It's just an assumption and I'll verify this. |
From: <ch...@su...> - 2013-08-23 11:39:26
|
Hi! > Thank you for this point. > > At first sight it seems that this testcase is very specific for i386 > (maybe for other architectures also). > > In i386 I found two variants of setgroups syscall (from > arch/x86/syscalls/syscall_32.tbl): > 81 i386 setgroups sys_setgroups16 > 206 i386 setgroups32 sys_setgroups > > I guess in i386 glibc handler for setgroups() internally uses > _setgroups32_ syscall. > > In x86_64 there is only one variant (from arch/x86/syscalls/syscall_64.tbl): > 116 common setgroups sys_setgroups > > which points to 32-bit version. > > But It's just an assumption and I'll verify this. The 16 variant is backward compatibility for old programs using this binari ABI so it makes sense that it does not exist on architectures that Linux were ported to after the 32 bit syscall was created. -- Cyril Hrubis ch...@su... |
From: Stanislav K. <sta...@or...> - 2013-08-26 10:51:23
|
On 08/23/2013 03:39 PM, ch...@su... wrote: > Hi! >> Thank you for this point. >> >> At first sight it seems that this testcase is very specific for i386 >> (maybe for other architectures also). >> >> In i386 I found two variants of setgroups syscall (from >> arch/x86/syscalls/syscall_32.tbl): >> 81 i386 setgroups sys_setgroups16 >> 206 i386 setgroups32 sys_setgroups >> >> I guess in i386 glibc handler for setgroups() internally uses >> _setgroups32_ syscall. >> >> In x86_64 there is only one variant (from arch/x86/syscalls/syscall_64.tbl): >> 116 common setgroups sys_setgroups >> >> which points to 32-bit version. >> >> But It's just an assumption and I'll verify this. > The 16 variant is backward compatibility for old programs using this > binari ABI so it makes sense that it does not exist on architectures > that Linux were ported to after the 32 bit syscall was created. > Hi. I looked at arch/* from linux sources and prepared this table: Architecture | __NR_setgroups32 defined | __NR_setgroups defined | glibc for setgroups() invokes | alpha | | + | arm | + | + | avr32 | | + | blackfin | + | + | cris | + | + | frv | + | + | h8300 | + | + | ia64 | + | | m32r | + | | m68k | + | + | microblaze | + | + | mips | | + | mn10300 | + | + | parisc | | + | powerpc | | + | setgroups | s390 | + | + | setgroups32 | s390x | | + | setgroups | sh-32 | + | + | setgroups32 | sh-64 | + | + | setgroups32 | sparc32 | + | + | setgroups32 | sparc64 | | + | setgroups | i386 | + | + | setgroups32 | x86_64 | | + | setgroups | xtensa | | + | Sorry for formatting. It looks like that if __NR_setgroups32 is defined then this means that syscall pointed by __NR_setgroups is 16bit. So how do you think about this patch ? diff --git a/testcases/kernel/syscalls/setgroups/compat_16.h b/testcases/kernel/syscalls/setgroups/compat_16.h index 0de4e78..8f46da9 100644 --- a/testcases/kernel/syscalls/setgroups/compat_16.h +++ b/testcases/kernel/syscalls/setgroups/compat_16.h @@ -32,9 +32,16 @@ extern void cleanup(void); #ifdef TST_USE_COMPAT16_SYSCALL long -SETGROUPS(size_t gidsetsize, GID_T *list) +SETGROUPS(size_t gidsetsize, GID_T *list16) { - return ltp_syscall(__NR_setgroups, gidsetsize, list); +# if (__NR_setgroups32 != __LTP__NR_INVALID_SYSCALL) + /* __NR_setgroups - 16-bit version of setgroups() syscall */ + return ltp_syscall(__NR_setgroups, gidsetsize, list16); +# else + /* The platform has no 16-bit version of setgroups() syscall */ + tst_brkm(TCONF, NULL, + "16-bit version of setgroups() is not supported on your arch"); +# endif /* __NR_setgroups32 */ } int |
From: <ch...@su...> - 2013-08-26 11:30:16
|
Hi! > I looked at arch/* from linux sources and prepared this table: > > Architecture | __NR_setgroups32 defined | __NR_setgroups defined | glibc > for setgroups() invokes | > alpha | | + | > arm | + | + | > avr32 | | + | > blackfin | + | + | > cris | + | + | > frv | + | + | > h8300 | + | + | > ia64 | + | | > m32r | + | | > m68k | + | + | > microblaze | + | + | > mips | | + | > mn10300 | + | + | > parisc | | + | > powerpc | | + | setgroups | > s390 | + | + | setgroups32 | > s390x | | + | setgroups | > sh-32 | + | + | setgroups32 | > sh-64 | + | + | setgroups32 | > sparc32 | + | + | setgroups32 | > sparc64 | | + | setgroups | > i386 | + | + | setgroups32 | > x86_64 | | + | setgroups | > xtensa | | + | > > Sorry for formatting. > > It looks like that if __NR_setgroups32 is defined then this means that > syscall pointed by __NR_setgroups is 16bit. > > So how do you think about this patch ? > > diff --git a/testcases/kernel/syscalls/setgroups/compat_16.h > b/testcases/kernel/syscalls/setgroups/compat_16.h > index 0de4e78..8f46da9 100644 > --- a/testcases/kernel/syscalls/setgroups/compat_16.h > +++ b/testcases/kernel/syscalls/setgroups/compat_16.h > @@ -32,9 +32,16 @@ extern void cleanup(void); > #ifdef TST_USE_COMPAT16_SYSCALL > > long > -SETGROUPS(size_t gidsetsize, GID_T *list) > +SETGROUPS(size_t gidsetsize, GID_T *list16) > { > - return ltp_syscall(__NR_setgroups, gidsetsize, list); > +# if (__NR_setgroups32 != __LTP__NR_INVALID_SYSCALL) > + /* __NR_setgroups - 16-bit version of setgroups() syscall */ > + return ltp_syscall(__NR_setgroups, gidsetsize, list16); > +# else > + /* The platform has no 16-bit version of setgroups() syscall */ > + tst_brkm(TCONF, NULL, > + "16-bit version of setgroups() is not supported on your > arch"); > +# endif /* __NR_setgroups32 */ Looks good to me. And we should fix the getgroups in the same header as well. -- Cyril Hrubis ch...@su... |
From: Stanislav K. <sta...@or...> - 2013-08-26 12:43:58
|
On 08/26/2013 03:30 PM, ch...@su... wrote: > Hi! >> I looked at arch/* from linux sources and prepared this table: >> >> Architecture | __NR_setgroups32 defined | __NR_setgroups defined | glibc >> for setgroups() invokes | >> alpha | | + | >> arm | + | + | >> avr32 | | + | >> blackfin | + | + | >> cris | + | + | >> frv | + | + | >> h8300 | + | + | >> ia64 | + | | >> m32r | + | | >> m68k | + | + | >> microblaze | + | + | >> mips | | + | >> mn10300 | + | + | >> parisc | | + | >> powerpc | | + | setgroups | >> s390 | + | + | setgroups32 | >> s390x | | + | setgroups | >> sh-32 | + | + | setgroups32 | >> sh-64 | + | + | setgroups32 | >> sparc32 | + | + | setgroups32 | >> sparc64 | | + | setgroups | >> i386 | + | + | setgroups32 | >> x86_64 | | + | setgroups | >> xtensa | | + | >> >> Sorry for formatting. >> >> It looks like that if __NR_setgroups32 is defined then this means that >> syscall pointed by __NR_setgroups is 16bit. >> >> So how do you think about this patch ? >> >> diff --git a/testcases/kernel/syscalls/setgroups/compat_16.h >> b/testcases/kernel/syscalls/setgroups/compat_16.h >> index 0de4e78..8f46da9 100644 >> --- a/testcases/kernel/syscalls/setgroups/compat_16.h >> +++ b/testcases/kernel/syscalls/setgroups/compat_16.h >> @@ -32,9 +32,16 @@ extern void cleanup(void); >> #ifdef TST_USE_COMPAT16_SYSCALL >> >> long >> -SETGROUPS(size_t gidsetsize, GID_T *list) >> +SETGROUPS(size_t gidsetsize, GID_T *list16) >> { >> - return ltp_syscall(__NR_setgroups, gidsetsize, list); >> +# if (__NR_setgroups32 != __LTP__NR_INVALID_SYSCALL) >> + /* __NR_setgroups - 16-bit version of setgroups() syscall */ >> + return ltp_syscall(__NR_setgroups, gidsetsize, list16); >> +# else >> + /* The platform has no 16-bit version of setgroups() syscall */ >> + tst_brkm(TCONF, NULL, >> + "16-bit version of setgroups() is not supported on your >> arch"); >> +# endif /* __NR_setgroups32 */ > Looks good to me. > > And we should fix the getgroups in the same header as well. > But we can't rely on the fact that group ids of a user invoking the testcases are fit into __old_gid_t. And the purpose of current GETGROUPS definition is to handle such cases. A situation may happen where root user is a member of a group with a _big_ group id. So I'm not 100% sure about fixing getgroups part of the same header. Or did I miss something? |
From: <ch...@su...> - 2013-08-26 14:16:07
|
Hi! > But we can't rely on the fact that group ids of a user invoking the > testcases are fit into __old_gid_t. > > And the purpose of current GETGROUPS definition is to handle such cases. > > A situation may happen where root user is a member of a group with a > _big_ group id. > > So I'm not 100% sure about fixing getgroups part of the same header. > > Or did I miss something? What I mean is that we should check if the compatibility getgroups syscall exists and if not abort the test (as the setgroups will do). Because the 32 bit getgroups syscall is tested with the tests without the TST_USE_COMPAT16_SYSCALL. -- Cyril Hrubis ch...@su... |
From: Stanislav K. <sta...@or...> - 2013-08-26 14:38:58
|
On 08/26/2013 06:16 PM, ch...@su... wrote: > Hi! >> But we can't rely on the fact that group ids of a user invoking the >> testcases are fit into __old_gid_t. >> >> And the purpose of current GETGROUPS definition is to handle such cases. >> >> A situation may happen where root user is a member of a group with a >> _big_ group id. >> >> So I'm not 100% sure about fixing getgroups part of the same header. >> >> Or did I miss something? > What I mean is that we should check if the compatibility getgroups > syscall exists and if not abort the test (as the setgroups will do). I understand, but should it be a part of syscalls/setgroups testcases? Maybe we should modify a little bit syscalls/getgroups tests and add this check there? > Because the 32 bit getgroups syscall is tested with the tests without > the TST_USE_COMPAT16_SYSCALL. |
From: Stanislav K. <sta...@or...> - 2013-08-27 09:35:46
|
getgroups01_16 and getgroups03_16 did not use 16-bit version of getgroups() syscall. Fixed this in the same manner as syscalls/setgroups. Signed-off-by: Stanislav Kholmanskikh <sta...@or...> --- testcases/kernel/syscalls/getgroups/compat_16.h | 78 +++++++++++++++++++++ testcases/kernel/syscalls/getgroups/getgroups01.c | 22 +++--- testcases/kernel/syscalls/getgroups/getgroups03.c | 17 +++-- 3 files changed, 99 insertions(+), 18 deletions(-) create mode 100644 testcases/kernel/syscalls/getgroups/compat_16.h diff --git a/testcases/kernel/syscalls/getgroups/compat_16.h b/testcases/kernel/syscalls/getgroups/compat_16.h new file mode 100644 index 0000000..79d006e --- /dev/null +++ b/testcases/kernel/syscalls/getgroups/compat_16.h @@ -0,0 +1,78 @@ +/* + * Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it would be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * Author: Stanislav Kholmanskikh <sta...@or...> + * + */ + +#ifndef __GETGROUPS_COMPAT_16_H__ +#define __GETGROUPS_COMPAT_16_H__ + +#include "compat_gid.h" +#include "linux_syscall_numbers.h" + +#ifdef TST_USE_COMPAT16_SYSCALL + +int SETGROUPS(size_t size, const GID_T *list16) +{ + int r; + int i; + + gid_t *list32; + list32 = calloc(size, sizeof(gid_t)); + if (list32 == NULL) + tst_brkm(TBROK | TERRNO, NULL, + "calloc() failed to allocate %zu bytes", + size * sizeof(gid_t)); + + for (i = 0; i < size; i++) + list32[i] = list16[i]; + + r = setgroups(size, list32); + + free(list32); + + return r; +} + +int GETGROUPS(int size, GID_T *list16) +{ +# if (__NR_getgroups32 != __LTP__NR_INVALID_SYSCALL) + /* __NR_getgroups - 16-bit version of getgroups() syscall */ + return ltp_syscall(__NR_getgroups, size, list16); +# else + /* The platform has no 16-bit version of getgroups() syscall */ + tst_brkm(TCONF, NULL, + "16-bit version of getgroups() is not supported on your arch"); +# endif /* __NR_getgroups32 */ +} + +#else + +int SETGROUPS(size_t size, const GID_T *list32) +{ + return setgroups(size, list32); +} + +int GETGROUPS(int size, GID_T *list32) +{ + return getgroups(size, list32); +} + +#endif /* TST_USE_COMPAT16_SYSCALL */ + +#endif /* __GETGROUPS_COMPAT_16_H__ */ diff --git a/testcases/kernel/syscalls/getgroups/getgroups01.c b/testcases/kernel/syscalls/getgroups/getgroups01.c index 1d58109..39157aa 100644 --- a/testcases/kernel/syscalls/getgroups/getgroups01.c +++ b/testcases/kernel/syscalls/getgroups/getgroups01.c @@ -52,8 +52,10 @@ #include <grp.h> #include <sys/param.h> #include <sys/types.h> + #include "test.h" #include "usctest.h" +#include "compat_16.h" static void setup(void); static void cleanup(void); @@ -61,14 +63,14 @@ static void cleanup(void); char *TCID = "getgroups01"; int TST_TOTAL = 4; -static gid_t gidset[NGROUPS]; -static gid_t cmpset[NGROUPS]; +static GID_T gidset[NGROUPS]; +static GID_T cmpset[NGROUPS]; int main(int ac, char **av) { int lc; char *msg; - gid_t group; + GID_T group; int i; int entries; @@ -83,7 +85,7 @@ int main(int ac, char **av) tst_count = 0; - TEST(getgroups(-1, gidset)); + TEST(GETGROUPS(-1, gidset)); if (TEST_RETURN == 0) { tst_resm(TFAIL, "getgroups succeeded unexpectedly"); @@ -101,14 +103,14 @@ int main(int ac, char **av) * return and the the gidset array is not modified. * This is a POSIX special case. */ - memset(gidset, 052, NGROUPS * sizeof(gid_t)); - memset(cmpset, 052, NGROUPS * sizeof(gid_t)); + memset(gidset, 052, NGROUPS * sizeof(GID_T)); + memset(cmpset, 052, NGROUPS * sizeof(GID_T)); - TEST(getgroups(0, gidset)); + TEST(GETGROUPS(0, gidset)); if (TEST_RETURN == -1) { tst_resm(TFAIL | TTERRNO, "getgroups failed unexpectedly"); } else if (STD_FUNCTIONAL_TEST) { - if (memcmp(cmpset, gidset, NGROUPS * sizeof(gid_t)) != 0) + if (memcmp(cmpset, gidset, NGROUPS * sizeof(GID_T)) != 0) tst_resm(TFAIL, "getgroups modified the gidset array"); else @@ -126,7 +128,7 @@ int main(int ac, char **av) "getgroups returned %ld; unable to test that using ngrps >=1 but less than number of grps", TEST_RETURN); } else { - TEST(getgroups(TEST_RETURN - 1, gidset)); + TEST(GETGROUPS(TEST_RETURN - 1, gidset)); if (TEST_RETURN == -1) { if (STD_FUNCTIONAL_TEST) { if (errno == EINVAL) @@ -145,7 +147,7 @@ int main(int ac, char **av) } } - TEST(getgroups(NGROUPS, gidset)); + TEST(GETGROUPS(NGROUPS, gidset)); if ((entries = TEST_RETURN) == -1) { tst_resm(TFAIL | TTERRNO, "getgroups failed unexpectedly"); diff --git a/testcases/kernel/syscalls/getgroups/getgroups03.c b/testcases/kernel/syscalls/getgroups/getgroups03.c index 1e25756..3a0ac0c 100644 --- a/testcases/kernel/syscalls/getgroups/getgroups03.c +++ b/testcases/kernel/syscalls/getgroups/getgroups03.c @@ -41,6 +41,7 @@ #include "test.h" #include "usctest.h" +#include "compat_16.h" #define TESTUSER "root" @@ -48,8 +49,8 @@ char *TCID = "getgroups03"; int TST_TOTAL = 1; static int ngroups; -static gid_t groups_list[NGROUPS]; -static gid_t groups[NGROUPS]; +static GID_T groups_list[NGROUPS]; +static GID_T groups[NGROUPS]; static void verify_groups(int ret_ngroups); static void setup(void); @@ -70,7 +71,7 @@ int main(int ac, char **av) tst_count = 0; - TEST(getgroups(gidsetsize, groups_list)); + TEST(GETGROUPS(gidsetsize, groups_list)); if (TEST_RETURN == -1) { tst_resm(TFAIL | TTERRNO, "getgroups failed"); @@ -88,18 +89,18 @@ int main(int ac, char **av) } /* - * readgroups(gid_t *) - Read supplimentary group ids of "root" user + * readgroups(GID_T *) - Read supplimentary group ids of "root" user * Scans the /etc/group file to get IDs of all the groups to which TESTUSER * belongs and puts them into the array passed. * Returns the no of gids read. */ -static int readgroups(gid_t groups[NGROUPS]) +static int readgroups(GID_T groups[NGROUPS]) { struct group *grp; int ngrps = 0; int i; int found; - gid_t g; + GID_T g; setgrent(); @@ -151,7 +152,7 @@ static void setup(void) * testcase will fail. So execute setgroups() before executing * getgroups() */ - if (setgroups(ngroups, groups) == -1) + if (SETGROUPS(ngroups, groups) == -1) tst_brkm(TBROK | TERRNO, cleanup, "setgroups failed"); } @@ -165,7 +166,7 @@ static void setup(void) static void verify_groups(int ret_ngroups) { int i, j; - gid_t egid; + GID_T egid; int egid_flag = 1; int fflag = 1; -- 1.7.1 |
From: Stanislav K. <sta...@or...> - 2013-08-27 09:35:46
|
Now 16-bit testcases are executed only if platform has corresponding 16-bit version of setgroups() syscall. Signed-off-by: Stanislav Kholmanskikh <sta...@or...> --- testcases/kernel/syscalls/setgroups/compat_16.h | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-) diff --git a/testcases/kernel/syscalls/setgroups/compat_16.h b/testcases/kernel/syscalls/setgroups/compat_16.h index 0de4e78..8f46da9 100644 --- a/testcases/kernel/syscalls/setgroups/compat_16.h +++ b/testcases/kernel/syscalls/setgroups/compat_16.h @@ -32,9 +32,16 @@ extern void cleanup(void); #ifdef TST_USE_COMPAT16_SYSCALL long -SETGROUPS(size_t gidsetsize, GID_T *list) +SETGROUPS(size_t gidsetsize, GID_T *list16) { - return ltp_syscall(__NR_setgroups, gidsetsize, list); +# if (__NR_setgroups32 != __LTP__NR_INVALID_SYSCALL) + /* __NR_setgroups - 16-bit version of setgroups() syscall */ + return ltp_syscall(__NR_setgroups, gidsetsize, list16); +# else + /* The platform has no 16-bit version of setgroups() syscall */ + tst_brkm(TCONF, NULL, + "16-bit version of setgroups() is not supported on your arch"); +# endif /* __NR_setgroups32 */ } int -- 1.7.1 |
From: <ch...@su...> - 2013-08-27 12:16:19
|
Hi! > Signed-off-by: Stanislav Kholmanskikh <sta...@or...> > --- > testcases/kernel/syscalls/getgroups/compat_16.h | 78 +++++++++++++++++++++ > testcases/kernel/syscalls/getgroups/getgroups01.c | 22 +++--- > testcases/kernel/syscalls/getgroups/getgroups03.c | 17 +++-- > 3 files changed, 99 insertions(+), 18 deletions(-) > create mode 100644 testcases/kernel/syscalls/getgroups/compat_16.h > > diff --git a/testcases/kernel/syscalls/getgroups/compat_16.h b/testcases/kernel/syscalls/getgroups/compat_16.h > new file mode 100644 > index 0000000..79d006e > --- /dev/null > +++ b/testcases/kernel/syscalls/getgroups/compat_16.h > @@ -0,0 +1,78 @@ > +/* > + * Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it would be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > + * > + * Author: Stanislav Kholmanskikh <sta...@or...> > + * > + */ > + > +#ifndef __GETGROUPS_COMPAT_16_H__ > +#define __GETGROUPS_COMPAT_16_H__ > + > +#include "compat_gid.h" > +#include "linux_syscall_numbers.h" > + > +#ifdef TST_USE_COMPAT16_SYSCALL > + > +int SETGROUPS(size_t size, const GID_T *list16) > +{ > + int r; > + int i; > + > + gid_t *list32; > + list32 = calloc(size, sizeof(gid_t)); > + if (list32 == NULL) > + tst_brkm(TBROK | TERRNO, NULL, > + "calloc() failed to allocate %zu bytes", > + size * sizeof(gid_t)); > + > + for (i = 0; i < size; i++) > + list32[i] = list16[i]; > + > + r = setgroups(size, list32); > + > + free(list32); > + > + return r; > +} I think that we should just use the 16 bit variant here as well, as well. It's available if the compat getgroups is and there is no need to call the 32 bit version. Or do you want to explicitly assert that the 32 bit syscall returns the same as the 16 bit? That would make some sense. > +int GETGROUPS(int size, GID_T *list16) > +{ > +# if (__NR_getgroups32 != __LTP__NR_INVALID_SYSCALL) > + /* __NR_getgroups - 16-bit version of getgroups() syscall */ > + return ltp_syscall(__NR_getgroups, size, list16); > +# else > + /* The platform has no 16-bit version of getgroups() syscall */ > + tst_brkm(TCONF, NULL, > + "16-bit version of getgroups() is not supported on your arch"); > +# endif /* __NR_getgroups32 */ > +} > + > +#else > + > +int SETGROUPS(size_t size, const GID_T *list32) > +{ > + return setgroups(size, list32); > +} > + > +int GETGROUPS(int size, GID_T *list32) > +{ > + return getgroups(size, list32); > +} > + > +#endif /* TST_USE_COMPAT16_SYSCALL */ > + > +#endif /* __GETGROUPS_COMPAT_16_H__ */ But still it duplicates code in the tree, lets move it into some header under 'LTP/include/sys/' and move compat_gid.h there as well. -- Cyril Hrubis ch...@su... |
From: Stanislav K. <sta...@or...> - 2013-08-28 06:44:32
|
On 08/27/2013 04:16 PM, ch...@su... wrote: > Hi! >> Signed-off-by: Stanislav Kholmanskikh <sta...@or...> >> --- >> testcases/kernel/syscalls/getgroups/compat_16.h | 78 +++++++++++++++++++++ >> testcases/kernel/syscalls/getgroups/getgroups01.c | 22 +++--- >> testcases/kernel/syscalls/getgroups/getgroups03.c | 17 +++-- >> 3 files changed, 99 insertions(+), 18 deletions(-) >> create mode 100644 testcases/kernel/syscalls/getgroups/compat_16.h >> >> diff --git a/testcases/kernel/syscalls/getgroups/compat_16.h b/testcases/kernel/syscalls/getgroups/compat_16.h >> new file mode 100644 >> index 0000000..79d006e >> --- /dev/null >> +++ b/testcases/kernel/syscalls/getgroups/compat_16.h >> @@ -0,0 +1,78 @@ >> +/* >> + * Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved. >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License as >> + * published by the Free Software Foundation; either version 2 of >> + * the License, or (at your option) any later version. >> + * >> + * This program is distributed in the hope that it would be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write the Free Software Foundation, >> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA >> + * >> + * Author: Stanislav Kholmanskikh <sta...@or...> >> + * >> + */ >> + >> +#ifndef __GETGROUPS_COMPAT_16_H__ >> +#define __GETGROUPS_COMPAT_16_H__ >> + >> +#include "compat_gid.h" >> +#include "linux_syscall_numbers.h" >> + >> +#ifdef TST_USE_COMPAT16_SYSCALL >> + >> +int SETGROUPS(size_t size, const GID_T *list16) >> +{ >> + int r; >> + int i; >> + >> + gid_t *list32; >> + list32 = calloc(size, sizeof(gid_t)); >> + if (list32 == NULL) >> + tst_brkm(TBROK | TERRNO, NULL, >> + "calloc() failed to allocate %zu bytes", >> + size * sizeof(gid_t)); >> + >> + for (i = 0; i < size; i++) >> + list32[i] = list16[i]; >> + >> + r = setgroups(size, list32); >> + >> + free(list32); >> + >> + return r; >> +} > I think that we should just use the 16 bit variant here as well, as > well. It's available if the compat getgroups is and there is no need to > call the 32 bit version. > > Or do you want to explicitly assert that the 32 bit syscall returns the > same as the 16 bit? That would make some sense. > >> +int GETGROUPS(int size, GID_T *list16) >> +{ >> +# if (__NR_getgroups32 != __LTP__NR_INVALID_SYSCALL) >> + /* __NR_getgroups - 16-bit version of getgroups() syscall */ >> + return ltp_syscall(__NR_getgroups, size, list16); >> +# else >> + /* The platform has no 16-bit version of getgroups() syscall */ >> + tst_brkm(TCONF, NULL, >> + "16-bit version of getgroups() is not supported on your arch"); >> +# endif /* __NR_getgroups32 */ >> +} >> + >> +#else >> + >> +int SETGROUPS(size_t size, const GID_T *list32) >> +{ >> + return setgroups(size, list32); >> +} >> + >> +int GETGROUPS(int size, GID_T *list32) >> +{ >> + return getgroups(size, list32); >> +} >> + >> +#endif /* TST_USE_COMPAT16_SYSCALL */ >> + >> +#endif /* __GETGROUPS_COMPAT_16_H__ */ > But still it duplicates code in the tree, lets move it into some header > under 'LTP/include/sys/' and move compat_gid.h there as well. > Hi! Ok. I'm managing this (compat_16.h from several testcases, compat_uid.h, compat_gid.h). But I need one more clarification. Look at LTP/testcases/kernel/include. There are linux_syscalls_numbers.h and numa_helper.h Maybe move all these compat headers not into LTP/include/sys, but into LTP/testcases/kernel/include? I think It's a more appropriate directory. How do you think? |
From: <ch...@su...> - 2013-08-28 09:54:58
|
Hi! > Ok. I'm managing this (compat_16.h from several testcases, compat_uid.h, > compat_gid.h). > > But I need one more clarification. > > Look at LTP/testcases/kernel/include. There are linux_syscalls_numbers.h > and numa_helper.h > > Maybe move all these compat headers not into LTP/include/sys, but into > LTP/testcases/kernel/include? > > I think It's a more appropriate directory. > > How do you think? I'm fine with testcases/kernel/include as well. -- Cyril Hrubis ch...@su... |
From: Stanislav K. <sta...@or...> - 2013-08-29 13:22:33
|
Hi! Changes since V2: * Fixed build dependencies handling in compat_16.mk * Moved compat_16.h to LTP/testcases/kernel/syscalls/utils. Because it already contains all supplemental headers and etc. * Now if we use a 16-bit syscall for set operation we should also use its 16-bit partner syscall for get operation (not 32-bit variant!). For example, setgroups/getgroups. * Tried to use defines in compat_16.h Please review. If It's ok I'm going to modify the same way the rest of 16-bit syscalls: getgid, getuid and etc. Most of them behaves incorrectly on 64bit systems. Thanks. |
From: Stanislav K. <sta...@or...> - 2013-08-29 13:22:37
|
getgroups01_16 and getgroups03_16 did not use 16-bit versions of getgroups() syscall. Fixed this in the same manner like syscalls/setgroups. Signed-off-by: Stanislav Kholmanskikh <sta...@or...> --- testcases/kernel/syscalls/getgroups/getgroups01.c | 22 +++++++++++--------- testcases/kernel/syscalls/getgroups/getgroups03.c | 17 ++++++++------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/testcases/kernel/syscalls/getgroups/getgroups01.c b/testcases/kernel/syscalls/getgroups/getgroups01.c index 1d58109..39157aa 100644 --- a/testcases/kernel/syscalls/getgroups/getgroups01.c +++ b/testcases/kernel/syscalls/getgroups/getgroups01.c @@ -52,8 +52,10 @@ #include <grp.h> #include <sys/param.h> #include <sys/types.h> + #include "test.h" #include "usctest.h" +#include "compat_16.h" static void setup(void); static void cleanup(void); @@ -61,14 +63,14 @@ static void cleanup(void); char *TCID = "getgroups01"; int TST_TOTAL = 4; -static gid_t gidset[NGROUPS]; -static gid_t cmpset[NGROUPS]; +static GID_T gidset[NGROUPS]; +static GID_T cmpset[NGROUPS]; int main(int ac, char **av) { int lc; char *msg; - gid_t group; + GID_T group; int i; int entries; @@ -83,7 +85,7 @@ int main(int ac, char **av) tst_count = 0; - TEST(getgroups(-1, gidset)); + TEST(GETGROUPS(-1, gidset)); if (TEST_RETURN == 0) { tst_resm(TFAIL, "getgroups succeeded unexpectedly"); @@ -101,14 +103,14 @@ int main(int ac, char **av) * return and the the gidset array is not modified. * This is a POSIX special case. */ - memset(gidset, 052, NGROUPS * sizeof(gid_t)); - memset(cmpset, 052, NGROUPS * sizeof(gid_t)); + memset(gidset, 052, NGROUPS * sizeof(GID_T)); + memset(cmpset, 052, NGROUPS * sizeof(GID_T)); - TEST(getgroups(0, gidset)); + TEST(GETGROUPS(0, gidset)); if (TEST_RETURN == -1) { tst_resm(TFAIL | TTERRNO, "getgroups failed unexpectedly"); } else if (STD_FUNCTIONAL_TEST) { - if (memcmp(cmpset, gidset, NGROUPS * sizeof(gid_t)) != 0) + if (memcmp(cmpset, gidset, NGROUPS * sizeof(GID_T)) != 0) tst_resm(TFAIL, "getgroups modified the gidset array"); else @@ -126,7 +128,7 @@ int main(int ac, char **av) "getgroups returned %ld; unable to test that using ngrps >=1 but less than number of grps", TEST_RETURN); } else { - TEST(getgroups(TEST_RETURN - 1, gidset)); + TEST(GETGROUPS(TEST_RETURN - 1, gidset)); if (TEST_RETURN == -1) { if (STD_FUNCTIONAL_TEST) { if (errno == EINVAL) @@ -145,7 +147,7 @@ int main(int ac, char **av) } } - TEST(getgroups(NGROUPS, gidset)); + TEST(GETGROUPS(NGROUPS, gidset)); if ((entries = TEST_RETURN) == -1) { tst_resm(TFAIL | TTERRNO, "getgroups failed unexpectedly"); diff --git a/testcases/kernel/syscalls/getgroups/getgroups03.c b/testcases/kernel/syscalls/getgroups/getgroups03.c index 1e25756..3a0ac0c 100644 --- a/testcases/kernel/syscalls/getgroups/getgroups03.c +++ b/testcases/kernel/syscalls/getgroups/getgroups03.c @@ -41,6 +41,7 @@ #include "test.h" #include "usctest.h" +#include "compat_16.h" #define TESTUSER "root" @@ -48,8 +49,8 @@ char *TCID = "getgroups03"; int TST_TOTAL = 1; static int ngroups; -static gid_t groups_list[NGROUPS]; -static gid_t groups[NGROUPS]; +static GID_T groups_list[NGROUPS]; +static GID_T groups[NGROUPS]; static void verify_groups(int ret_ngroups); static void setup(void); @@ -70,7 +71,7 @@ int main(int ac, char **av) tst_count = 0; - TEST(getgroups(gidsetsize, groups_list)); + TEST(GETGROUPS(gidsetsize, groups_list)); if (TEST_RETURN == -1) { tst_resm(TFAIL | TTERRNO, "getgroups failed"); @@ -88,18 +89,18 @@ int main(int ac, char **av) } /* - * readgroups(gid_t *) - Read supplimentary group ids of "root" user + * readgroups(GID_T *) - Read supplimentary group ids of "root" user * Scans the /etc/group file to get IDs of all the groups to which TESTUSER * belongs and puts them into the array passed. * Returns the no of gids read. */ -static int readgroups(gid_t groups[NGROUPS]) +static int readgroups(GID_T groups[NGROUPS]) { struct group *grp; int ngrps = 0; int i; int found; - gid_t g; + GID_T g; setgrent(); @@ -151,7 +152,7 @@ static void setup(void) * testcase will fail. So execute setgroups() before executing * getgroups() */ - if (setgroups(ngroups, groups) == -1) + if (SETGROUPS(ngroups, groups) == -1) tst_brkm(TBROK | TERRNO, cleanup, "setgroups failed"); } @@ -165,7 +166,7 @@ static void setup(void) static void verify_groups(int ret_ngroups) { int i, j; - gid_t egid; + GID_T egid; int egid_flag = 1; int fflag = 1; -- 1.7.1 |
From: <ch...@su...> - 2013-09-02 15:28:46
|
Hi! > getgroups01_16 and getgroups03_16 did not use 16-bit > versions of getgroups() syscall. Fixed this in the same manner like > syscalls/setgroups. This part looks fine. -- Cyril Hrubis ch...@su... |
From: Stanislav K. <sta...@or...> - 2013-08-29 13:22:38
|
syscalls/setgroups: moved compat_16.h to ../utils. This header will be common for all 16-bit syscalls definitions. Signed-off-by: Stanislav Kholmanskikh <sta...@or...> --- testcases/kernel/syscalls/setgroups/compat_16.h | 85 ---------------------- testcases/kernel/syscalls/utils/compat_16.h | 89 +++++++++++++++++++++++ 2 files changed, 89 insertions(+), 85 deletions(-) delete mode 100644 testcases/kernel/syscalls/setgroups/compat_16.h create mode 100644 testcases/kernel/syscalls/utils/compat_16.h diff --git a/testcases/kernel/syscalls/setgroups/compat_16.h b/testcases/kernel/syscalls/setgroups/compat_16.h deleted file mode 100644 index 0de4e78..0000000 --- a/testcases/kernel/syscalls/setgroups/compat_16.h +++ /dev/null @@ -1,85 +0,0 @@ -/* - * - * Copyright (c) Red Hat Inc., 2008 - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; either version 2 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See - * the GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA - */ - -/* Author: Masatake YAMATO <ya...@re...> */ - -#ifndef __SETGROUPS_COMPAT_16_H__ -#define __SETGROUPS_COMPAT_16_H__ - -#include "compat_gid.h" -#include "linux_syscall_numbers.h" - - -/* For avoiding circular dependency. */ -extern void cleanup(void); - -#ifdef TST_USE_COMPAT16_SYSCALL - -long -SETGROUPS(size_t gidsetsize, GID_T *list) -{ - return ltp_syscall(__NR_setgroups, gidsetsize, list); -} - -int -GETGROUPS(int size16, GID_T *list16) -{ - int r; - int i; - - gid_t *list32; - - list32 = malloc(size16 * sizeof(gid_t)); - if (list32 == NULL) - tst_brkm(TBROK, NULL, "malloc failed to alloc %zu errno " - " %d ", size16 * sizeof(gid_t), errno); - - r = getgroups(size16, list32); - if (r < 0) - goto out; - - for (i = 0; i < r; i++) { - if (!GID_SIZE_CHECK(list32[i])) - tst_brkm(TBROK, - cleanup, - "gid returned from getgroups is too large for testing setgroups32"); - list16[i] = (GID_T)list32[i]; - } - - out: - free(list32); - return r; -} - -#else -int -SETGROUPS(size_t size, const GID_T *list) -{ - return setgroups(size, list); -} - -int -GETGROUPS(int size, GID_T *list) -{ - return getgroups(size, list); -} - -#endif /* TST_USE_COMPAT16_SYSCALL */ - -#endif /* __SETGROUPS_COMPAT_16_H__ */ diff --git a/testcases/kernel/syscalls/utils/compat_16.h b/testcases/kernel/syscalls/utils/compat_16.h new file mode 100644 index 0000000..dc5dafa --- /dev/null +++ b/testcases/kernel/syscalls/utils/compat_16.h @@ -0,0 +1,89 @@ +/* + * Copyright (c) Red Hat Inc., 2008 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See + * the GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/* Author: Masatake YAMATO <ya...@re...> */ + +#ifndef __SETGROUPS_COMPAT_16_H__ +#define __SETGROUPS_COMPAT_16_H__ + +#include "compat_gid.h" +#include "linux_syscall_numbers.h" + + +/* For avoiding circular dependency. */ +static void cleanup(void); + +#ifdef TST_USE_COMPAT16_SYSCALL + +/* + * LTP_COMPAT16_IS_DEFINED(SNAME) - if true, then __NR_SNAME + * is a 16-bit version of SNAME() syscall + * + * LTP_COMPAT16_IF_DEFINED(SNAME, ...) - action to execute + * if 16-bit version of SNAME syscall is supported + * + * LTP_COMPAT16_IF_UNDEFINED(SNAME) - action to execute + * if 16-bit version of SNAME syscall is not supported + */ +#define LTP_COMPAT16_IS_DEFINED(SNAME) \ + (__NR_##SNAME##32 != __LTP__NR_INVALID_SYSCALL) + +#define LTP_COMPAT16_IF_DEFINED(SNAME, ...) \ + return ltp_syscall(__NR_##SNAME, ##__VA_ARGS__); + +#define LTP_COMPAT16_IF_UNDEFINED(SNAME) \ + tst_brkm(TCONF, cleanup, \ + "16-bit version of %s() is not supported on your platform", \ + #SNAME); + +int +SETGROUPS(size_t gidsetsize, GID_T *list16) +{ +# if LTP_COMPAT16_IS_DEFINED(setgroups) + LTP_COMPAT16_IF_DEFINED(setgroups, gidsetsize, list16) +# else + LTP_COMPAT16_IF_UNDEFINED(setgroups) +# endif +} + +int +GETGROUPS(int gidsetsize, GID_T *list16) +{ +# if LTP_COMPAT16_IS_DEFINED(getgroups) + LTP_COMPAT16_IF_DEFINED(getgroups, gidsetsize, list16) +# else + LTP_COMPAT16_IF_UNDEFINED(getgroups) +# endif +} + +#else +int +SETGROUPS(size_t size, const GID_T *list) +{ + return setgroups(size, list); +} + +int +GETGROUPS(int size, GID_T *list) +{ + return getgroups(size, list); +} + +#endif /* TST_USE_COMPAT16_SYSCALL */ + +#endif /* __SETGROUPS_COMPAT_16_H__ */ -- 1.7.1 |
From: Stanislav K. <sta...@or...> - 2013-08-29 13:22:35
|
If we create/modify/touch file syscalls/utils/compat_16.h and execute 'make' in any of syscalls directories which includes compat_16.mk, then nothing will happen, because this approach: %.c: $(COMPAT_16_H) is not working. Fixed this. Signed-off-by: Stanislav Kholmanskikh <sta...@or...> --- testcases/kernel/syscalls/utils/compat_16.mk | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/testcases/kernel/syscalls/utils/compat_16.mk b/testcases/kernel/syscalls/utils/compat_16.mk index 371bd43..36ece1b 100644 --- a/testcases/kernel/syscalls/utils/compat_16.mk +++ b/testcases/kernel/syscalls/utils/compat_16.mk @@ -55,6 +55,7 @@ CPPFLAGS += -I$(abs_srcdir) -I$(abs_srcdir)/../utils SRCS ?= $(wildcard $(abs_srcdir)/*.c) MAKE_TARGETS := $(notdir $(patsubst %.c,%,$(SRCS))) +MAKE_TARGETS_OBJS_WO_COMPAT_16 := $(addsuffix .o,$(MAKE_TARGETS)) ifneq ($(TST_COMPAT_16_SYSCALL),no) MAKE_TARGETS += $(addsuffix _16,$(MAKE_TARGETS)) @@ -69,7 +70,8 @@ COMPAT_16_H := $(abs_srcdir)/../utils/compat_16.h ifneq ($(wildcard $(COMPAT_16_H)),) HAS_COMPAT_16 := 1 -%.c: $(COMPAT_16_H) +$(MAKE_TARGETS_OBJS_WO_COMPAT_16): $(COMPAT_16_H) +.INTERMEDIATE: $(MAKE_TARGETS_OBJS_WO_COMPAT_16) else HAS_COMPAT_16 := 0 @@ -78,5 +80,5 @@ endif %_16: CPPFLAGS += -D$(DEF_16)=1 # XXX (garrcoop): End section of code in question.. -%_16.o: %.c +%_16.o: %.c $(COMPAT_16_H) $(COMPILE.c) $(OUTPUT_OPTION) $< -- 1.7.1 |
From: <ch...@su...> - 2013-09-02 15:22:53
|
> +/* > + * LTP_COMPAT16_IS_DEFINED(SNAME) - if true, then __NR_SNAME > + * is a 16-bit version of SNAME() syscall > + * > + * LTP_COMPAT16_IF_DEFINED(SNAME, ...) - action to execute > + * if 16-bit version of SNAME syscall is supported > + * > + * LTP_COMPAT16_IF_UNDEFINED(SNAME) - action to execute > + * if 16-bit version of SNAME syscall is not supported > + */ > +#define LTP_COMPAT16_IS_DEFINED(SNAME) \ > + (__NR_##SNAME##32 != __LTP__NR_INVALID_SYSCALL) > + > +#define LTP_COMPAT16_IF_DEFINED(SNAME, ...) \ > + return ltp_syscall(__NR_##SNAME, ##__VA_ARGS__); > + > +#define LTP_COMPAT16_IF_UNDEFINED(SNAME) \ > + tst_brkm(TCONF, cleanup, \ > + "16-bit version of %s() is not supported on your platform", \ > + #SNAME); What I do not like is the calling cleanup from this part of the code, it was not passed there and the code will break badly if you name the cleanup function differently (or create more of them, cleanup, cleanup1, ...). What I would do here is to set errno to ENOSYS and return a failure. And let the test that called the function handle it. Or we can add a cleanup parameter to the all caps functions bellow. > +int > +SETGROUPS(size_t gidsetsize, GID_T *list16) > +{ > +# if LTP_COMPAT16_IS_DEFINED(setgroups) > + LTP_COMPAT16_IF_DEFINED(setgroups, gidsetsize, list16) > +# else > + LTP_COMPAT16_IF_UNDEFINED(setgroups) > +# endif > +} > + > +int > +GETGROUPS(int gidsetsize, GID_T *list16) > +{ > +# if LTP_COMPAT16_IS_DEFINED(getgroups) > + LTP_COMPAT16_IF_DEFINED(getgroups, gidsetsize, list16) > +# else > + LTP_COMPAT16_IF_UNDEFINED(getgroups) > +# endif > +} > + > +#else > +int > +SETGROUPS(size_t size, const GID_T *list) > +{ > + return setgroups(size, list); > +} > + > +int > +GETGROUPS(int size, GID_T *list) > +{ > + return getgroups(size, list); > +} > + > +#endif /* TST_USE_COMPAT16_SYSCALL */ > + > +#endif /* __SETGROUPS_COMPAT_16_H__ */ We can even simplify this macros. At least the innter part of SETGROUPS and GETGROUPS could be created by a single macro: COMPAT16_SYSCALL(setgroups, gitsetsize, list16) #define COMPAT16_SYSCALL(sys_name, ...) \ # ifdef TST_USE_COMPAT16_SYSCALL # if .... return ltp_syscall(sys_name, ##__VA_ARGS__); # else errno = ENOSYS; return -1; # fi # else return sys_name(__VA_ARGS__); # endif In case that the syscall and libcall params does not match we can use only the part enclosed in the ifdef TST_USE_COMPAT16_SYSCALL. -- Cyril Hrubis ch...@su... |
From: <ch...@su...> - 2013-09-02 15:59:18
|
Hi! > If we create/modify/touch file syscalls/utils/compat_16.h and execute > 'make' in any of syscalls directories which includes compat_16.mk, then > nothing will happen, because this approach: > > %.c: $(COMPAT_16_H) > > is not working. > > Fixed this. > > Signed-off-by: Stanislav Kholmanskikh <sta...@or...> > --- > testcases/kernel/syscalls/utils/compat_16.mk | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/testcases/kernel/syscalls/utils/compat_16.mk b/testcases/kernel/syscalls/utils/compat_16.mk > index 371bd43..36ece1b 100644 > --- a/testcases/kernel/syscalls/utils/compat_16.mk > +++ b/testcases/kernel/syscalls/utils/compat_16.mk > @@ -55,6 +55,7 @@ CPPFLAGS += -I$(abs_srcdir) -I$(abs_srcdir)/../utils > SRCS ?= $(wildcard $(abs_srcdir)/*.c) > > MAKE_TARGETS := $(notdir $(patsubst %.c,%,$(SRCS))) > +MAKE_TARGETS_OBJS_WO_COMPAT_16 := $(addsuffix .o,$(MAKE_TARGETS)) > > ifneq ($(TST_COMPAT_16_SYSCALL),no) > MAKE_TARGETS += $(addsuffix _16,$(MAKE_TARGETS)) > @@ -69,7 +70,8 @@ COMPAT_16_H := $(abs_srcdir)/../utils/compat_16.h > ifneq ($(wildcard $(COMPAT_16_H)),) > HAS_COMPAT_16 := 1 > > -%.c: $(COMPAT_16_H) > +$(MAKE_TARGETS_OBJS_WO_COMPAT_16): $(COMPAT_16_H) > +.INTERMEDIATE: $(MAKE_TARGETS_OBJS_WO_COMPAT_16) > > else > HAS_COMPAT_16 := 0 > @@ -78,5 +80,5 @@ endif > %_16: CPPFLAGS += -D$(DEF_16)=1 > # XXX (garrcoop): End section of code in question.. > > -%_16.o: %.c > +%_16.o: %.c $(COMPAT_16_H) > $(COMPILE.c) $(OUTPUT_OPTION) $< Perhaps we can do this more cleanly by making the _16 binaries depend on the compat_16.h as with: --- a/testcases/kernel/syscalls/utils/compat_16.mk +++ b/testcases/kernel/syscalls/utils/compat_16.mk @@ -57,7 +57,8 @@ SRCS ?= $(wildcard $(abs_srcdir)/*.c) MAKE_TARGETS := $(notdir $(patsubst %.c,%,$(SRCS))) ifneq ($(TST_COMPAT_16_SYSCALL),no) -MAKE_TARGETS += $(addsuffix _16,$(MAKE_TARGETS)) +MAKE_TARGETS_16 = $(addsuffix _16,$(MAKE_TARGETS)) +MAKE_TARGETS += $(MAKE_TARGETS_16) endif # XXX (garrcoop): This code should be put in question as it cannot be applied @@ -69,8 +70,7 @@ COMPAT_16_H := $(abs_srcdir)/../utils/compat_16.h ifneq ($(wildcard $(COMPAT_16_H)),) HAS_COMPAT_16 := 1 -%.c: $(COMPAT_16_H) - +$(MAKE_TARGETS_16): $(COMPAT_16_H) else HAS_COMPAT_16 := 0 endif -- Cyril Hrubis ch...@su... |
From: Stanislav K. <sta...@or...> - 2013-09-03 06:11:00
|
On 09/02/2013 07:59 PM, ch...@su... wrote: > Hi! Hi! >> If we create/modify/touch file syscalls/utils/compat_16.h and execute >> 'make' in any of syscalls directories which includes compat_16.mk, then >> nothing will happen, because this approach: >> >> %.c: $(COMPAT_16_H) >> >> is not working. >> >> Fixed this. >> >> Signed-off-by: Stanislav Kholmanskikh <sta...@or...> >> --- >> testcases/kernel/syscalls/utils/compat_16.mk | 6 ++++-- >> 1 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/testcases/kernel/syscalls/utils/compat_16.mk b/testcases/kernel/syscalls/utils/compat_16.mk >> index 371bd43..36ece1b 100644 >> --- a/testcases/kernel/syscalls/utils/compat_16.mk >> +++ b/testcases/kernel/syscalls/utils/compat_16.mk >> @@ -55,6 +55,7 @@ CPPFLAGS += -I$(abs_srcdir) -I$(abs_srcdir)/../utils >> SRCS ?= $(wildcard $(abs_srcdir)/*.c) >> >> MAKE_TARGETS := $(notdir $(patsubst %.c,%,$(SRCS))) >> +MAKE_TARGETS_OBJS_WO_COMPAT_16 := $(addsuffix .o,$(MAKE_TARGETS)) >> >> ifneq ($(TST_COMPAT_16_SYSCALL),no) >> MAKE_TARGETS += $(addsuffix _16,$(MAKE_TARGETS)) >> @@ -69,7 +70,8 @@ COMPAT_16_H := $(abs_srcdir)/../utils/compat_16.h >> ifneq ($(wildcard $(COMPAT_16_H)),) >> HAS_COMPAT_16 := 1 >> >> -%.c: $(COMPAT_16_H) >> +$(MAKE_TARGETS_OBJS_WO_COMPAT_16): $(COMPAT_16_H) >> +.INTERMEDIATE: $(MAKE_TARGETS_OBJS_WO_COMPAT_16) >> >> else >> HAS_COMPAT_16 := 0 >> @@ -78,5 +80,5 @@ endif >> %_16: CPPFLAGS += -D$(DEF_16)=1 >> # XXX (garrcoop): End section of code in question.. >> >> -%_16.o: %.c >> +%_16.o: %.c $(COMPAT_16_H) >> $(COMPILE.c) $(OUTPUT_OPTION) $< > Perhaps we can do this more cleanly by making the _16 binaries depend on > the compat_16.h as with: > > --- a/testcases/kernel/syscalls/utils/compat_16.mk > +++ b/testcases/kernel/syscalls/utils/compat_16.mk > @@ -57,7 +57,8 @@ SRCS ?= $(wildcard $(abs_srcdir)/*.c) > MAKE_TARGETS := $(notdir $(patsubst %.c,%,$(SRCS))) > > ifneq ($(TST_COMPAT_16_SYSCALL),no) > -MAKE_TARGETS += $(addsuffix _16,$(MAKE_TARGETS)) > +MAKE_TARGETS_16 = $(addsuffix _16,$(MAKE_TARGETS)) > +MAKE_TARGETS += $(MAKE_TARGETS_16) > endif > > # XXX (garrcoop): This code should be put in question as it cannot be applied > @@ -69,8 +70,7 @@ COMPAT_16_H := $(abs_srcdir)/../utils/compat_16.h > ifneq ($(wildcard $(COMPAT_16_H)),) > HAS_COMPAT_16 := 1 > > -%.c: $(COMPAT_16_H) > - > +$(MAKE_TARGETS_16): $(COMPAT_16_H) > else > HAS_COMPAT_16 := 0 > endif > Not only _16 binaries should depend on compat_16.h, but binaries w/o _16 too. So in that case we need to use: +$(MAKE_TARGETS): $(COMPAT_16_H) instead of: +$(MAKE_TARGETS_16): $(COMPAT_16_H) And definition of MAKE_TARGETS_16 is not needed at all. So the final variant is as follows: --- a/testcases/kernel/syscalls/utils/compat_16.mk +++ b/testcases/kernel/syscalls/utils/compat_16.mk @@ -69,8 +69,7 @@ COMPAT_16_H := $(abs_srcdir)/../utils/compat_16.h ifneq ($(wildcard $(COMPAT_16_H)),) HAS_COMPAT_16 := 1 -%.c: $(COMPAT_16_H) - +$(MAKE_TARGETS): $(COMPAT_16_H) + else HAS_COMPAT_16 := 0 endif But I think that my approach is easier to understand: we explicitly define object files targets, make them depend on compat_16.h and also mark them as intermediate targets (so make will remove them after linking). And if we make binaries to depend on compat_16.h gcc treats this header file not as a header: gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -I/home/stas/ltp/testcases/kernel/include -I/home/stas/ltp/testcases/kernel/syscalls/setgroups -I/home/stas/ltp/testcases/kernel/syscalls/setgroups/../utils -I../../../../include -I../../../../include -L../../../../lib setgroups01.c /home/stas/ltp/testcases/kernel/syscalls/setgroups/../utils/compat_16.h -lltp -o setgroups01 gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -I/home/stas/ltp/testcases/kernel/include -I/home/stas/ltp/testcases/kernel/syscalls/setgroups -I/home/stas/ltp/testcases/kernel/syscalls/setgroups/../utils -I../../../../include -I../../../../include -DTST_USE_COMPAT16_SYSCALL=1 -c -o setgroups01_16.o setgroups01.c gcc -L../../../../lib setgroups01_16.o /home/stas/ltp/testcases/kernel/syscalls/setgroups/../utils/compat_16.h -lltp -o setgroups01_16 It doesn't look nice :) On the other side if we make object files to depend on compat_16.h gcc treats this header file as a header: gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -I/home/stas/ltp/testcases/kernel/include -I/home/stas/ltp/testcases/kernel/syscalls/setgroups -I/home/stas/ltp/testcases/kernel/syscalls/setgroups/../utils -I../../../../include -I../../../../include -c -o setgroups01.o setgroups01.c gcc -L../../../../lib setgroups01.o -lltp -o setgroups01 gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -I/home/stas/ltp/testcases/kernel/include -I/home/stas/ltp/testcases/kernel/syscalls/setgroups -I/home/stas/ltp/testcases/kernel/syscalls/setgroups/../utils -I../../../../include -I../../../../include -DTST_USE_COMPAT16_SYSCALL=1 -c -o setgroups01_16.o setgroups01.c gcc -L../../../../lib setgroups01_16.o -lltp -o setgroups01_16 |
From: <ch...@su...> - 2013-09-03 11:06:44
|
Hi! > But I think that my approach is easier to understand: we explicitly > define object files targets, make them depend on compat_16.h and also > mark them as intermediate targets (so make will remove them after linking). > > And if we make binaries to depend on compat_16.h gcc treats this header > file not as a header: > > gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall > -I/home/stas/ltp/testcases/kernel/include > -I/home/stas/ltp/testcases/kernel/syscalls/setgroups > -I/home/stas/ltp/testcases/kernel/syscalls/setgroups/../utils > -I../../../../include -I../../../../include -L../../../../lib > setgroups01.c > /home/stas/ltp/testcases/kernel/syscalls/setgroups/../utils/compat_16.h > -lltp -o setgroups01 > > gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall > -I/home/stas/ltp/testcases/kernel/include > -I/home/stas/ltp/testcases/kernel/syscalls/setgroups > -I/home/stas/ltp/testcases/kernel/syscalls/setgroups/../utils > -I../../../../include -I../../../../include > -DTST_USE_COMPAT16_SYSCALL=1 -c -o setgroups01_16.o setgroups01.c > gcc -L../../../../lib setgroups01_16.o > /home/stas/ltp/testcases/kernel/syscalls/setgroups/../utils/compat_16.h > -lltp -o setgroups01_16 > > It doesn't look nice :) Ah, it gets to the list of prerequisities and gets apended to the linking. I Should have noticed this. I guess that your approach is better. -- Cyril Hrubis ch...@su... |
From: Stanislav K. <sta...@or...> - 2013-09-03 08:04:48
|
On 09/02/2013 07:23 PM, ch...@su... wrote: > We can even simplify this macros. At least the innter part of SETGROUPS > and GETGROUPS could be created by a single macro: > > COMPAT16_SYSCALL(setgroups, gitsetsize, list16) > > #define COMPAT16_SYSCALL(sys_name, ...) \ > # ifdef TST_USE_COMPAT16_SYSCALL > # if .... > return ltp_syscall(sys_name, ##__VA_ARGS__); > # else > errno = ENOSYS; > return -1; > # fi > # else > return sys_name(__VA_ARGS__); > # endif > > In case that the syscall and libcall params does not match we can use > only the part enclosed in the ifdef TST_USE_COMPAT16_SYSCALL. Sorry, but is it possible to use #ifdef or #if inside a #define macro? This doesn't work: #define MYDEF(NN) \ # ifdef TST_USE_COMPAT16_SYSCALL return NN(0); # else return NN(1); # endif MYDEF(bla) Could you point to a working example, please? |
From: <ch...@su...> - 2013-09-03 10:11:08
|
Hi! > Sorry, but is it possible to use #ifdef or #if inside a #define macro? Right it does not, sorry. I tend to forget how dumb it the C preprocessor is. > This doesn't work: > > #define MYDEF(NN) \ > # ifdef TST_USE_COMPAT16_SYSCALL > return NN(0); > # else > return NN(1); > # endif > > MYDEF(bla) We need to pull the first if out of the macro and we can do the second one as a regular if (it will be optimized away anyway). #ifdef TST_USE_COMPAT16_SYSCALL # define LTP_CREATE_SYSCALL(sys_name, ...) \ if (__NR_##sys_name##32 != __LTP__NR_INVALID_SYSCALL) { \ return ltp_syscall(sys_name, ##__VA_ARGS__); \ } else { \ errno = ENOSYS; \ return -1; \ } #else # define LTP_CREATE_SYSCALL(sys_name, ...) \ return sys_name(__VA_ARGS__) #endif The usage will be: int SETGROUPS(size_t size, const GID_T *list) { LTP_CREATE_SYSCALL(getgroups, size, list); } Does this sound reasonable? -- Cyril Hrubis ch...@su... |