syscalls/sendto02.c: add new testcase
PATCH 1/1 syscalls/sendto02.c: add new testcase
When sctp protocol is selected in socket(2) and buffer is invalid, sendto(2) should fail and set errno to EFAULT, but it sets errno to ENOMEM.
This is a regression test and has been fixed by kernel patch:
'commit 6e51fe757259 ("sctp: fix -ENOMEM result with invalid user space pointer in sendto() syscall")'
--- runtest/syscalls | 1 + testcases/kernel/syscalls/.gitignore | 1 + testcases/kernel/syscalls/sendto/sendto02.c | 126 ++++++++++++++++++++++++++++ 3 files changed, 128 insertions(+) create mode 100644 testcases/kernel/syscalls/sendto/sendto02.c
runtest/syscalls +1 -0
@@ -1005,6 +1005,7 @@ sendmsg01 sendmsg01 sendmsg02 sendmsg02 sendto01 sendto01 +sendto02 sendto02 setdomainname01 setdomainname01 setdomainname02 setdomainname02
testcases/kernel/syscalls/.gitignore +1 -0
@@ -814,6 +814,7 @@ /sendmsg/sendmsg01 /sendmsg/sendmsg02 /sendto/sendto01 +/sendto/sendto02 /set_robust_list/set_robust_list01 /set_thread_area/set_thread_area01 /set_thread_area/set_thread_area02
testcases/kernel/syscalls/sendto/sendto02.c new file +126 -0
126 lines not on this page · raw message
Signature · 1 line
1.8.3.1
PATCH 1/1 syscalls/sendto02.c: add new testcase
Hi!
Quoted 8 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +8
-0+#include <errno.h> +#include <sys/types.h> +#include <sys/socket.h> + +#include "tst_test.h" + +static int sockfd; +static int rds_flag;
testcases/kernel/syscalls/sendto/sendto02.c line 47
Quoted 10 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +10
-0+static struct sockaddr_in sa; + +static void setup (void ) +{ +int acc_res, load_res; +const char *cmd[] = {"modprobe" ,"sctp" ,NULL }; + + acc_res =access ("/proc/sys/net/sctp" ,F_OK ); +if (acc_res ==-1 && errno !=ENOENT ) +tst_brk (TFAIL |TERRNO ,"failed to check stcp module" );
^
I would make the error message as specific as possible here, so I would print what exactly has failed:
tst_brk(TBROK | TERRNO, "access(/proc/sys/net/sctp, F_OK)");
testcases/kernel/syscalls/sendto/sendto02.c line 52
Quoted 4 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +4
-0+if (acc_res ==-1 && errno ==ENOENT ) { + load_res =tst_run_cmd (cmd,NULL ,NULL ,1 ); +if (load_res) { + tst_brk(TCONF, "failed to loaded sctp module, "
^ load
testcases/kernel/syscalls/sendto/sendto02.c line 53
Quoted 1 line
testcases/kernel/syscalls/sendto/sendto02.c new file +1
-0+ "so sctp modeule was not support by system");
^ module
Also I would say that all that needs to be printed here is the first part of the string, i.e. "failed to load sctp module".
testcases/kernel/syscalls/sendto/sendto02.c line 57
Quoted 4 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +4
-0+ } + +tst_res (TINFO ,"succeeded to load sctp module" ); + rds_flag =1 ;
^
It would be a bit better to name it rds_module_loaded or just module_loaded
testcases/kernel/syscalls/sendto/sendto02.c line 60
Quoted 3 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +3
-0+ } + +tst_res (TINFO ,"sctp module was supported by system" );
Just omit this message, it's misleading anyway.
testcases/kernel/syscalls/sendto/sendto02.c line 62
Quoted 1 line
testcases/kernel/syscalls/sendto/sendto02.c new file +1
-0+ sockfd =SAFE_SOCKET (AF_INET ,SOCK_STREAM ,132 /*IPPROTO_SCTP*/ );
^
This is way too ugly.
If IPPROTO_SCTP is not supported on you system you should add a fallback definition such as:
#ifndef IPPROTO_SCTP
# define IPPROTO_SCTP 132 #endif
Also you didn't even try to include the <netinet/in.h> header that contains this definition, so it's quite likely that all you need to do is to include this header as well.
Quoted 13 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +13
-0+memset (&sa,0 ,sizeof (sa)); + sa.sin_family =AF_INET ; + sa.sin_addr.s_addr =inet_addr ("127.0.0.1" ); + sa.sin_port =htons (11111 ); +} + +static void cleanup (void ) +{ +int unload_res; +const char *cmd[] = {"modprobe" ,"-r" ,"sctp" ,NULL }; + +if (rds_flag ==1 ) { + unload_res =tst_run_cmd (cmd,NULL ,NULL ,1 );
testcases/kernel/syscalls/sendto/sendto02.c line 86
Quoted 10 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +10
-0+if (unload_res) { +/* We failed to unload sctp module(modprobe -r sctp) +* and the operation failed with "FATAL: Module sctp is +* in use." lsmod shows a reference count of 2 for sctp. +* If we rebuild kernel with CONFIG_MODULE_FORCE_UNLOAD +* enabled, we can succeed to unload sctp module +* (rmmod -f sctp). +*/ +tst_res (TINFO ,"failed to unload sctp module, you can" +" reboot system to unload sctp after testing" );
Isn't the problem here that you have to close the sctp socket *berofe* you try to remove the module?
It's pretty clear that the module would be in use at least until the socked is closed.
Quoted 10 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +10
-0+ }else { +tst_res (TINFO ,"succeeded to unload sctp modules" ); + } + } + +if (sockfd >0 &&close (sockfd)) +tst_res (TWARN |TERRNO ,"failed to close file" ); +} + +static void verify_sendto (void )
testcases/kernel/syscalls/sendto/sendto02.c line 106
Quoted 10 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +10
-0+{ +TEST (sendto (sockfd,NULL ,1 ,0 , (struct sockaddr *) &sa,sizeof (sa))); +if (TEST_RETURN !=-1 ) { +tst_res (TFAIL ,"sendto() succeeded unexpectedly" ); +return ; + } + +if (TEST_ERRNO ==ENOMEM ) { +tst_res (TFAIL |TTERRNO ,"sendto() got unrepaired errno with" +" invalid buffer when sctp is selected in socket()" );
This message is absurdly long as well. Be short and to the point.
Something as:
tst_res(TFAIL | TTERRNO, "sendto(fd, NULL, ...) failed unexpectedly");
testcases/kernel/syscalls/sendto/sendto02.c line 112
Quoted 6 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +6
-0+return ; + } + +if (TEST_ERRNO ==EFAULT ) { +tst_res (TPASS |TTERRNO ,"sendto() got repaired errno with" +" invalid buffer when sctp is selected in socket()" );
Here as well. Shorten the message to something more reasonable.
testcases/kernel/syscalls/sendto/sendto02.c line 118
Quoted 6 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +6
-0+return ; + } + +tst_res (TFAIL |TTERRNO ,"sendto() got unexpected errno with invalid" +" buffer when sctp is selected in socket(), expected ENOMEM " +"or EFAULT" );
Why do we have special case for errno != ENOMEM && errno != EFAULT. We fail the testcase if the errno is not EFAULT anyway. There is no reason to complicate the code like this.
Quoted 16 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +8
-0+} + +static struct tst_test test = { + .tid ="sendto02" , + .setup = setup, + .cleanup = cleanup, + .test_all = verify_sendto, +};Signature · 7 lines
1.8.3.1 -- Mailing list info: https://lists.linux.it/listinfo/ltp
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 syscalls/sendto02.c: add new testcase
Hi cyril
Thanks for your review.
I will rewrite this pacth as you said, but i still have some doubts. Please see the following comment.
Quoted 91 lines · Cyril Hrubis
Hi!
Quoted 8 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +8
-0+#include <errno.h> +#include <sys/types.h> +#include <sys/socket.h> + +#include "tst_test.h" + +static int sockfd; +static int rds_flag;testcases/kernel/syscalls/sendto/sendto02.c line 47
Quoted 10 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +10
-0+static struct sockaddr_in sa; + +static void setup (void ) +{ +int acc_res, load_res; +const char *cmd[] = {"modprobe" ,"sctp" ,NULL }; + + acc_res =access ("/proc/sys/net/sctp" ,F_OK ); +if (acc_res ==-1 && errno !=ENOENT ) +tst_brk (TFAIL |TERRNO ,"failed to check stcp module" );^I would make the error message as specific as possible here, so I would print what exactly has failed:
tst_brk(TBROK | TERRNO, "access(/proc/sys/net/sctp, F_OK)");testcases/kernel/syscalls/sendto/sendto02.c line 52
Quoted 4 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +4
-0+if (acc_res ==-1 && errno ==ENOENT ) { + load_res =tst_run_cmd (cmd,NULL ,NULL ,1 ); +if (load_res) { + tst_brk(TCONF, "failed to loaded sctp module, "^ loadtestcases/kernel/syscalls/sendto/sendto02.c line 53
Quoted 1 line
testcases/kernel/syscalls/sendto/sendto02.c new file +1
-0+ "so sctp modeule was not support by system");^ moduleAlso I would say that all that needs to be printed here is the first part of the string, i.e. "failed to load sctp module".
testcases/kernel/syscalls/sendto/sendto02.c line 57
Quoted 4 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +4
-0+ } + +tst_res (TINFO ,"succeeded to load sctp module" ); + rds_flag =1 ;^
It would be a bit better to name itrds_module_loadedor justmodule_loadedtestcases/kernel/syscalls/sendto/sendto02.c line 60
Quoted 3 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +3
-0+ } + +tst_res (TINFO ,"sctp module was supported by system" );Just omit this message, it's misleading anyway.
testcases/kernel/syscalls/sendto/sendto02.c line 62
Quoted 1 line
testcases/kernel/syscalls/sendto/sendto02.c new file +1
-0+ sockfd =SAFE_SOCKET (AF_INET ,SOCK_STREAM ,132 /*IPPROTO_SCTP*/ );^
This is way too ugly.If
IPPROTO_SCTPis not supported on you system you should add a fallback definition such as:#ifndef
IPPROTO_SCTP# define IPPROTO_SCTP 132 #endifAlso you didn't even try to include the<netinet/in.h> header that contains this definition, so it's quite likely that all you need to do is to include this header as well.
Quoted 13 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +13
-0+memset (&sa,0 ,sizeof (sa)); + sa.sin_family =AF_INET ; + sa.sin_addr.s_addr =inet_addr ("127.0.0.1" ); + sa.sin_port =htons (11111 ); +} + +static void cleanup (void ) +{ +int unload_res; +const char *cmd[] = {"modprobe" ,"-r" ,"sctp" ,NULL }; + +if (rds_flag ==1 ) { + unload_res =tst_run_cmd (cmd,NULL ,NULL ,1 );testcases/kernel/syscalls/sendto/sendto02.c line 86
Quoted 10 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +10
-0+if (unload_res) { +/* We failed to unload sctp module(modprobe -r sctp) +* and the operation failed with "FATAL: Module sctp is +* in use." lsmod shows a reference count of 2 for sctp. +* If we rebuild kernel with CONFIG_MODULE_FORCE_UNLOAD +* enabled, we can succeed to unload sctp module +* (rmmod -f sctp). +*/ +tst_res (TINFO ,"failed to unload sctp module, you can" +" reboot system to unload sctp after testing" );Isn't the problem here that you have to close the sctp socket *berofe* you try to remove the module?
Quoted 3 lines
It's pretty clear that the module would be in use at least until the socked is closed.
this is not the problem here. the module was in use even we have closed sctp socket before removing the module.
the sctp module was in use because of some references. Please see the following url for detailed information:
http://www.spinics.net/lists/linux-sctp/msg02137.html
Thanks,
Xiao Yang
Quoted 59 lines
Quoted 10 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +10
-0+ }else { +tst_res (TINFO ,"succeeded to unload sctp modules" ); + } + } + +if (sockfd>0 &&close (sockfd)) +tst_res (TWARN |TERRNO ,"failed to close file" ); +} + +static void verify_sendto (void )testcases/kernel/syscalls/sendto/sendto02.c line 106
Quoted 10 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +10
-0+{ +TEST (sendto (sockfd,NULL ,1 ,0 , (struct sockaddr *)&sa,sizeof (sa))); +if (TEST_RETURN !=-1 ) { +tst_res (TFAIL ,"sendto() succeeded unexpectedly" ); +return ; + } + +if (TEST_ERRNO ==ENOMEM ) { +tst_res (TFAIL |TTERRNO ,"sendto() got unrepaired errno with" +" invalid buffer when sctp is selected in socket()" );This message is absurdly long as well. Be short and to the point.
Something as:
tst_res(TFAIL | TTERRNO, "sendto(fd, NULL, ...) failed unexpectedly");testcases/kernel/syscalls/sendto/sendto02.c line 112
Quoted 6 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +6
-0+return ; + } + +if (TEST_ERRNO ==EFAULT ) { +tst_res (TPASS |TTERRNO ,"sendto() got repaired errno with" +" invalid buffer when sctp is selected in socket()" );Here as well. Shorten the message to something more reasonable.
testcases/kernel/syscalls/sendto/sendto02.c line 118
Quoted 6 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +6
-0+return ; + } + +tst_res (TFAIL |TTERRNO ,"sendto() got unexpected errno with invalid" +" buffer when sctp is selected in socket(), expected ENOMEM " +"or EFAULT" );Why do we have special case for errno != ENOMEM&& errno != EFAULT. We fail the testcase if the errno is not EFAULT anyway. There is no reason to complicate the code like this.
Quoted 16 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +8
-0+} + +static struct tst_test test = { + .tid ="sendto02" , + .setup = setup, + .cleanup = cleanup, + .test_all = verify_sendto, +};Signature · 7 lines
1.8.3.1 -- Mailing list info: https://lists.linux.it/listinfo/ltp
PATCH v2 1/1 syscalls/sendto02.c: add new testcase
In reply to Cyril HrubisWhen sctp protocol is selected in socket(2) and buffer is invalid, sendto(2) should fail and set errno to EFAULT, but it sets errno to ENOMEM.
This is a regression test and has been fixed by kernel patch:
'commit 6e51fe757259 ("sctp: fix -ENOMEM result with invalid user space pointer in sendto() syscall")'
--- runtest/syscalls | 1 + testcases/kernel/syscalls/.gitignore | 1 + testcases/kernel/syscalls/sendto/sendto02.c | 91 +++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+) create mode 100644 testcases/kernel/syscalls/sendto/sendto02.c
runtest/syscalls +1 -0
@@ -1006,6 +1006,7 @@ sendmsg01 sendmsg01 sendmsg02 sendmsg02 sendto01 sendto01 +sendto02 sendto02 setdomainname01 setdomainname01 setdomainname02 setdomainname02
testcases/kernel/syscalls/.gitignore +1 -0
@@ -814,6 +814,7 @@ /sendmsg/sendmsg01 /sendmsg/sendmsg02 /sendto/sendto01 +/sendto/sendto02 /set_robust_list/set_robust_list01 /set_thread_area/set_thread_area01 /set_thread_area/set_thread_area02
testcases/kernel/syscalls/sendto/sendto02.c new file +91 -0
91 lines not on this page · raw message
Signature · 1 line
1.8.3.1
PATCH v2 1/1 syscalls/sendto02.c: add new testcase
Hi!
Pushed, thanks.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 syscalls/sendto02.c: add new testcase
In reply to Cyril HrubisHi cyril
I find that system would load sctp module when calling scoket(..., IPPROTO_SCTP).
If sctp module is not supported by system, it returns EPROTONOSUPPORT. I will remove loading/unloading code.
Thanks,
Xiao Yang
Quoted 154 lines · Cyril Hrubis
Hi!
Quoted 8 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +8
-0+#include <errno.h> +#include <sys/types.h> +#include <sys/socket.h> + +#include "tst_test.h" + +static int sockfd; +static int rds_flag;testcases/kernel/syscalls/sendto/sendto02.c line 47
Quoted 10 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +10
-0+static struct sockaddr_in sa; + +static void setup (void ) +{ +int acc_res, load_res; +const char *cmd[] = {"modprobe" ,"sctp" ,NULL }; + + acc_res =access ("/proc/sys/net/sctp" ,F_OK ); +if (acc_res ==-1 && errno !=ENOENT ) +tst_brk (TFAIL |TERRNO ,"failed to check stcp module" );^I would make the error message as specific as possible here, so I would print what exactly has failed:
tst_brk(TBROK | TERRNO, "access(/proc/sys/net/sctp, F_OK)");testcases/kernel/syscalls/sendto/sendto02.c line 52
Quoted 4 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +4
-0+if (acc_res ==-1 && errno ==ENOENT ) { + load_res =tst_run_cmd (cmd,NULL ,NULL ,1 ); +if (load_res) { + tst_brk(TCONF, "failed to loaded sctp module, "^ loadtestcases/kernel/syscalls/sendto/sendto02.c line 53
Quoted 1 line
testcases/kernel/syscalls/sendto/sendto02.c new file +1
-0+ "so sctp modeule was not support by system");^ moduleAlso I would say that all that needs to be printed here is the first part of the string, i.e. "failed to load sctp module".
testcases/kernel/syscalls/sendto/sendto02.c line 57
Quoted 4 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +4
-0+ } + +tst_res (TINFO ,"succeeded to load sctp module" ); + rds_flag =1 ;^
It would be a bit better to name itrds_module_loadedor justmodule_loadedtestcases/kernel/syscalls/sendto/sendto02.c line 60
Quoted 3 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +3
-0+ } + +tst_res (TINFO ,"sctp module was supported by system" );Just omit this message, it's misleading anyway.
testcases/kernel/syscalls/sendto/sendto02.c line 62
Quoted 1 line
testcases/kernel/syscalls/sendto/sendto02.c new file +1
-0+ sockfd =SAFE_SOCKET (AF_INET ,SOCK_STREAM ,132 /*IPPROTO_SCTP*/ );^
This is way too ugly.If
IPPROTO_SCTPis not supported on you system you should add a fallback definition such as:#ifndef
IPPROTO_SCTP# define IPPROTO_SCTP 132 #endifAlso you didn't even try to include the<netinet/in.h> header that contains this definition, so it's quite likely that all you need to do is to include this header as well.
Quoted 13 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +13
-0+memset (&sa,0 ,sizeof (sa)); + sa.sin_family =AF_INET ; + sa.sin_addr.s_addr =inet_addr ("127.0.0.1" ); + sa.sin_port =htons (11111 ); +} + +static void cleanup (void ) +{ +int unload_res; +const char *cmd[] = {"modprobe" ,"-r" ,"sctp" ,NULL }; + +if (rds_flag ==1 ) { + unload_res =tst_run_cmd (cmd,NULL ,NULL ,1 );testcases/kernel/syscalls/sendto/sendto02.c line 86
Quoted 10 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +10
-0+if (unload_res) { +/* We failed to unload sctp module(modprobe -r sctp) +* and the operation failed with "FATAL: Module sctp is +* in use." lsmod shows a reference count of 2 for sctp. +* If we rebuild kernel with CONFIG_MODULE_FORCE_UNLOAD +* enabled, we can succeed to unload sctp module +* (rmmod -f sctp). +*/ +tst_res (TINFO ,"failed to unload sctp module, you can" +" reboot system to unload sctp after testing" );Isn't the problem here that you have to close the sctp socket *berofe* you try to remove the module?
It's pretty clear that the module would be in use at least until the socked is closed.
Quoted 10 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +10
-0+ }else { +tst_res (TINFO ,"succeeded to unload sctp modules" ); + } + } + +if (sockfd>0 &&close (sockfd)) +tst_res (TWARN |TERRNO ,"failed to close file" ); +} + +static void verify_sendto (void )testcases/kernel/syscalls/sendto/sendto02.c line 106
Quoted 10 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +10
-0+{ +TEST (sendto (sockfd,NULL ,1 ,0 , (struct sockaddr *)&sa,sizeof (sa))); +if (TEST_RETURN !=-1 ) { +tst_res (TFAIL ,"sendto() succeeded unexpectedly" ); +return ; + } + +if (TEST_ERRNO ==ENOMEM ) { +tst_res (TFAIL |TTERRNO ,"sendto() got unrepaired errno with" +" invalid buffer when sctp is selected in socket()" );This message is absurdly long as well. Be short and to the point.
Something as:
tst_res(TFAIL | TTERRNO, "sendto(fd, NULL, ...) failed unexpectedly");testcases/kernel/syscalls/sendto/sendto02.c line 112
Quoted 6 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +6
-0+return ; + } + +if (TEST_ERRNO ==EFAULT ) { +tst_res (TPASS |TTERRNO ,"sendto() got repaired errno with" +" invalid buffer when sctp is selected in socket()" );Here as well. Shorten the message to something more reasonable.
testcases/kernel/syscalls/sendto/sendto02.c line 118
Quoted 6 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +6
-0+return ; + } + +tst_res (TFAIL |TTERRNO ,"sendto() got unexpected errno with invalid" +" buffer when sctp is selected in socket(), expected ENOMEM " +"or EFAULT" );Why do we have special case for errno != ENOMEM&& errno != EFAULT. We fail the testcase if the errno is not EFAULT anyway. There is no reason to complicate the code like this.
Quoted 16 lines
testcases/kernel/syscalls/sendto/sendto02.c new file +8
-0+} + +static struct tst_test test = { + .tid ="sendto02" , + .setup = setup, + .cleanup = cleanup, + .test_all = verify_sendto, +};Signature · 7 lines
1.8.3.1 -- Mailing list info: https://lists.linux.it/listinfo/ltp
PATCH 1/1 syscalls/sendto02.c: add new testcase
Hi!
Quoted 4 lines
I find that system would load sctp module when calling
scoket(..., IPPROTO_SCTP).
If sctp module is not supported by system, it returns EPROTONOSUPPORT. I will remove loading/unloading code.
I started to wonder about that as I looked at the v2 patch. Sounds better than loading/unloading the module explicitly.
Signature · 2 lines
Cyril Hrubis [email protected]