| Message ID | 20210806025659.1962902-1-liwang@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [COMMITTED] setsockopt08: includes netinet/in.h | expand |
Hi Li, > We have to put netinet/in.h on the top to get rid of conflict > of glibc and kernel headers for old unbuntu. > ----------- > /usr/include/linux/in.h:28:3: error: redeclaration of enumerator 'IPPROTO_IP' > IPPROTO_IP = 0, /* Dummy protocol for TCP */ > ^ > /usr/include/netinet/in.h:42:5: note: previous definition of 'IPPROTO_IP' was here > IPPROTO_IP = 0, /* Dummy protocol for TCP. */ > ... > ----------- > See: https://www.mail-archive.com/netdev@vger.kernel.org/msg132711.html Thanks for fixing it, it's not a first time we got hit by this. I wonder where <linux/in.h> is included. It's not directly in setsockopt08.c, it must be in our lapi header. But it's not in tst_safe_net.h, not in safe_net_fn.h nor in tst_net.h and both actually include <netinet/in.h>, thus it must be before. But there is only tst_test.h. I'm asking because it'd be better to add <netinet/in.h> into header before <linux/in.h>. Kind regards, Petr > Fixes: ebf3a4fbd39a (Add setsockopt08, CVE-2021-22555) > Signed-off-by: Li Wang <liwang@redhat.com> > --- > testcases/kernel/syscalls/setsockopt/setsockopt08.c | 2 ++ > 1 file changed, 2 insertions(+) > diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt08.c b/testcases/kernel/syscalls/setsockopt/setsockopt08.c > index f758dcbdc..f7052f27b 100644 > --- a/testcases/kernel/syscalls/setsockopt/setsockopt08.c > +++ b/testcases/kernel/syscalls/setsockopt/setsockopt08.c > @@ -79,6 +79,8 @@ > * - sizeof(struct xt_entry_target) = 32 > */ > +#include <netinet/in.h> > + > #include "tst_test.h" > #include "tst_safe_net.h" > #include "lapi/ip_tables.h"
Hi Li, Cyril, > Hi Li, > > We have to put netinet/in.h on the top to get rid of conflict > > of glibc and kernel headers for old unbuntu. > > ----------- > > /usr/include/linux/in.h:28:3: error: redeclaration of enumerator 'IPPROTO_IP' > > IPPROTO_IP = 0, /* Dummy protocol for TCP */ > > ^ > > /usr/include/netinet/in.h:42:5: note: previous definition of 'IPPROTO_IP' was here > > IPPROTO_IP = 0, /* Dummy protocol for TCP. */ > > ... > > ----------- > > See: https://www.mail-archive.com/netdev@vger.kernel.org/msg132711.html > Thanks for fixing it, it's not a first time we got hit by this. > I wonder where <linux/in.h> is included. It's not directly in setsockopt08.c, > it must be in our lapi header. But it's not in tst_safe_net.h, not in > safe_net_fn.h nor in tst_net.h and both actually include <netinet/in.h>, thus it must be > before. But there is only tst_test.h. > I'm asking because it'd be better to add <netinet/in.h> into header before > <linux/in.h>. OK, it's in lapi/ip_tables.h, which includes <linux/netfilter_ipv4/ip_tables.h> which includes <linux/if.h>. But I wonder why inclusion of <netinet/in.h> from safe_net_fn.h or in tst_net.h does not work. Anyway, I'll test including <netinet/in.h> in include/lapi/ip_tables.h helps. Kind regards, Petr > Kind regards, > Petr > > Fixes: ebf3a4fbd39a (Add setsockopt08, CVE-2021-22555) > > Signed-off-by: Li Wang <liwang@redhat.com> > > --- > > testcases/kernel/syscalls/setsockopt/setsockopt08.c | 2 ++ > > 1 file changed, 2 insertions(+) > > diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt08.c b/testcases/kernel/syscalls/setsockopt/setsockopt08.c > > index f758dcbdc..f7052f27b 100644 > > --- a/testcases/kernel/syscalls/setsockopt/setsockopt08.c > > +++ b/testcases/kernel/syscalls/setsockopt/setsockopt08.c > > @@ -79,6 +79,8 @@ > > * - sizeof(struct xt_entry_target) = 32 > > */ > > +#include <netinet/in.h> > > + > > #include "tst_test.h" > > #include "tst_safe_net.h" > > #include "lapi/ip_tables.h"
Hi Petr, > > > See: > https://www.mail-archive.com/netdev@vger.kernel.org/msg132711.html > > > Thanks for fixing it, it's not a first time we got hit by this. > > I wonder where <linux/in.h> is included. It's not directly in > setsockopt08.c, > > it must be in our lapi header. But it's not in tst_safe_net.h, not in > > safe_net_fn.h nor in tst_net.h and both actually include <netinet/in.h>, > thus it must be > > before. But there is only tst_test.h. > > > I'm asking because it'd be better to add <netinet/in.h> into header > before > > <linux/in.h>. > > OK, it's in lapi/ip_tables.h, which includes > <linux/netfilter_ipv4/ip_tables.h> > which includes <linux/if.h>. But I wonder why inclusion of <netinet/in.h> > from > No, it's not caused by the lapi/ip_tables.h which finally includes <linux/if.h>. See experiment commit: https://github.com/wangli5665/ltp/commit/f1a37712c63472b19d3355446fb66e651b4a186e The conflict happened early in tst_test.h and I guess some header files between line#14 to line#44 probably involves <linux/if.h>, but I'm not sure which one is the culprit. If we simply put the <netinet/in.h> at the top of tst_test.h, the conflict disappears as well. See experiment commit: https://github.com/wangli5665/ltp/commit/0155df479811d9a51f30e09accb330238607f73d $ cat include/tst_test.h -n ... 14 #include <unistd.h> 15 #include <limits.h> 16 #include <string.h> 17 #include <errno.h> 18 19 #include "tst_common.h" 20 #include "tst_res_flags.h" 21 #include "tst_test_macros.h" 22 #include "tst_checkpoint.h" 23 #include "tst_device.h" 24 #include "tst_mkfs.h" 25 #include "tst_fs.h" 26 #include "tst_pid.h" 27 #include "tst_cmd.h" 28 #include "tst_cpu.h" 29 #include "tst_process_state.h" 30 #include "tst_atomic.h" 31 #include "tst_kvercmp.h" 32 #include "tst_kernel.h" 33 #include "tst_minmax.h" 34 #include "tst_get_bad_addr.h" 35 #include "tst_path_has_mnt_flags.h" 36 #include "tst_sys_conf.h" 37 #include "tst_coredump.h" 38 #include "tst_buffers.h" 39 #include "tst_capability.h" 40 #include "tst_hugepage.h" 41 #include "tst_assert.h" 42 #include "tst_lockdown.h" 43 #include "tst_fips.h" 44 #include "tst_taint.h" ... 93 #include "tst_safe_macros.h" 94 #include "tst_safe_file_ops.h" 95 #include "tst_safe_net.h" <===== includes the <netinet/in.h> here 96 #include "tst_clone.h"
Hi Li, > Hi Petr, > > > > See: > > https://www.mail-archive.com/netdev@vger.kernel.org/msg132711.html > > > Thanks for fixing it, it's not a first time we got hit by this. > > > I wonder where <linux/in.h> is included. It's not directly in > > setsockopt08.c, > > > it must be in our lapi header. But it's not in tst_safe_net.h, not in > > > safe_net_fn.h nor in tst_net.h and both actually include <netinet/in.h>, > > thus it must be > > > before. But there is only tst_test.h. > > > I'm asking because it'd be better to add <netinet/in.h> into header > > before > > > <linux/in.h>. > > OK, it's in lapi/ip_tables.h, which includes > > <linux/netfilter_ipv4/ip_tables.h> > > which includes <linux/if.h>. But I wonder why inclusion of <netinet/in.h> > > from > No, it's not caused by the lapi/ip_tables.h which finally includes > <linux/if.h>. > See experiment commit: > https://github.com/wangli5665/ltp/commit/f1a37712c63472b19d3355446fb66e651b4a186e Yep, I also found myself it does not help. > The conflict happened early in tst_test.h and I guess some header files > between line#14 to line#44 probably involves <linux/if.h>, but I'm not sure > which one is the culprit. Interesting, really something in in tst_test.h with combination of lapi/ip_tables.h. This combination is only in 2 tests, setsockopt0{3,8}.c, setsockopt03.c already had <netinet/in.h>. > If we simply put the <netinet/in.h> at the top of tst_test.h, the > conflict disappears > as well. > See experiment commit: > https://github.com/wangli5665/ltp/commit/0155df479811d9a51f30e09accb330238607f73d I'd be for adding it there, with comment why it's there. We can prevent problems with failing another test in the future. (+ remove it from both tests). Kind regards, Petr > $ cat include/tst_test.h -n > ... > 14 #include <unistd.h> > 15 #include <limits.h> > 16 #include <string.h> > 17 #include <errno.h> > 18 > 19 #include "tst_common.h" > 20 #include "tst_res_flags.h" > 21 #include "tst_test_macros.h" > 22 #include "tst_checkpoint.h" > 23 #include "tst_device.h" > 24 #include "tst_mkfs.h" > 25 #include "tst_fs.h" > 26 #include "tst_pid.h" > 27 #include "tst_cmd.h" > 28 #include "tst_cpu.h" > 29 #include "tst_process_state.h" > 30 #include "tst_atomic.h" > 31 #include "tst_kvercmp.h" > 32 #include "tst_kernel.h" > 33 #include "tst_minmax.h" > 34 #include "tst_get_bad_addr.h" > 35 #include "tst_path_has_mnt_flags.h" > 36 #include "tst_sys_conf.h" > 37 #include "tst_coredump.h" > 38 #include "tst_buffers.h" > 39 #include "tst_capability.h" > 40 #include "tst_hugepage.h" > 41 #include "tst_assert.h" > 42 #include "tst_lockdown.h" > 43 #include "tst_fips.h" > 44 #include "tst_taint.h" > ... > 93 #include "tst_safe_macros.h" > 94 #include "tst_safe_file_ops.h" > 95 #include "tst_safe_net.h" <===== includes the <netinet/in.h> here > 96 #include "tst_clone.h"
> > The conflict happened early in tst_test.h and I guess some header files > > between line#14 to line#44 probably involves <linux/if.h>, but I'm not > sure > > which one is the culprit. > Interesting, really something in in tst_test.h with combination of > lapi/ip_tables.h. This combination is only in 2 tests, setsockopt0{3,8}.c, > setsockopt03.c already had <netinet/in.h>. > I eventually caught that "tst_capability.h" is the key point. To includes <netinet/in.h> before that can avoid the conflict. But still not sure how it made things broken (i.e. where includes <linux/if.h>). > > If we simply put the <netinet/in.h> at the top of tst_test.h, the > > conflict disappears > > as well. > > See experiment commit: > > > https://github.com/wangli5665/ltp/commit/0155df479811d9a51f30e09accb330238607f73d > > I'd be for adding it there, with comment why it's there. We can prevent > problems > with failing another test in the future. (+ remove it from both tests). > I'm OK with this fix. @Cyril, @Richard, what do you think? any other thoughts?
Hell Li, Li Wang <liwang@redhat.com> writes: >> > The conflict happened early in tst_test.h and I guess some header files >> > between line#14 to line#44 probably involves <linux/if.h>, but I'm not >> sure >> > which one is the culprit. >> Interesting, really something in in tst_test.h with combination of >> lapi/ip_tables.h. This combination is only in 2 tests, setsockopt0{3,8}.c, >> setsockopt03.c already had <netinet/in.h>. >> > > I eventually caught that "tst_capability.h" is the key point. > To includes <netinet/in.h> before that can avoid the conflict. > > But still not sure how it made things broken (i.e. where includes > <linux/if.h>). > > >> > If we simply put the <netinet/in.h> at the top of tst_test.h, the >> > conflict disappears >> > as well. >> > See experiment commit: >> > >> https://github.com/wangli5665/ltp/commit/0155df479811d9a51f30e09accb330238607f73d >> >> I'd be for adding it there, with comment why it's there. We can prevent >> problems >> with failing another test in the future. (+ remove it from both tests). >> > > I'm OK with this fix. > > @Cyril, @Richard, what do you think? any other thoughts? We need to clean up our headers, which is a bigger problem. Most tests do not need all the stuff in tst_test.h. It is just a load of unecessary work. Cleaning up the headers is a big challenge. It would be easier if we know what will break older distros. So I suggest adding something like: #ifdef _X_H # error "You should include X before Y ..." #endif to one or more headers. Otherwise I'm fine with the above solution as a "temporary" fix.
Hi Richard, > >> I'd be for adding it there, with comment why it's there. We can prevent > >> problems > >> with failing another test in the future. (+ remove it from both tests). > >> > > > > I'm OK with this fix. > > > > @Cyril, @Richard, what do you think? any other thoughts? > > We need to clean up our headers, which is a bigger problem. Most tests > do not need all the stuff in tst_test.h. It is just a load of unecessary > work. > Yes, I agree. It benefits for long-term maintenance of LTP. So maybe a good start is to be more strict in review new patches especially on the includes header check. > > Cleaning up the headers is a big challenge. It would be easier if we > know what will break older distros. So I suggest adding something like: > > #ifdef _X_H > # error "You should include X before Y ..." > #endif > > Hmm, it's right, but I'm afraid we have to define that in many places because include headers are sometimes dispersive or singly. I.e. We need to define the above hint duplicated for everywhere if includes the <netinet/in.h>, to avoid include <linux/in.h> before it. This will look a bit messy I feel. $ git grep netinet/in.h lapi/netinet_in.h:#include <netinet/in.h> old/old_safe_net.h:#include <netinet/in.h> safe_net_fn.h:#include <netinet/in.h> tst_net.h:#include <netinet/in.h> tst_safe_net.h:#include <netinet/in.h> > to one or more headers. > > Otherwise I'm fine with the above solution as a "temporary" fix. >
> Hell Li, > Li Wang <liwang@redhat.com> writes: > >> > The conflict happened early in tst_test.h and I guess some header files > >> > between line#14 to line#44 probably involves <linux/if.h>, but I'm not > >> sure > >> > which one is the culprit. > >> Interesting, really something in in tst_test.h with combination of > >> lapi/ip_tables.h. This combination is only in 2 tests, setsockopt0{3,8}.c, > >> setsockopt03.c already had <netinet/in.h>. > > I eventually caught that "tst_capability.h" is the key point. > > To includes <netinet/in.h> before that can avoid the conflict. > > But still not sure how it made things broken (i.e. where includes > > <linux/if.h>). > >> > If we simply put the <netinet/in.h> at the top of tst_test.h, the > >> > conflict disappears > >> > as well. > >> > See experiment commit: > >> https://github.com/wangli5665/ltp/commit/0155df479811d9a51f30e09accb330238607f73d > >> I'd be for adding it there, with comment why it's there. We can prevent > >> problems > >> with failing another test in the future. (+ remove it from both tests). > > I'm OK with this fix. > > @Cyril, @Richard, what do you think? any other thoughts? > We need to clean up our headers, which is a bigger problem. Most tests > do not need all the stuff in tst_test.h. It is just a load of unecessary > work. > Cleaning up the headers is a big challenge. It would be easier if we > know what will break older distros. So I suggest adding something like: > #ifdef _X_H > # error "You should include X before Y ..." > #endif +1 for this. > to one or more headers. > Otherwise I'm fine with the above solution as a "temporary" fix. quotation marks in "temporary" are correct, "temporary" means forever :). Kind regards, Petr
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt08.c b/testcases/kernel/syscalls/setsockopt/setsockopt08.c index f758dcbdc..f7052f27b 100644 --- a/testcases/kernel/syscalls/setsockopt/setsockopt08.c +++ b/testcases/kernel/syscalls/setsockopt/setsockopt08.c @@ -79,6 +79,8 @@ * - sizeof(struct xt_entry_target) = 32 */ +#include <netinet/in.h> + #include "tst_test.h" #include "tst_safe_net.h" #include "lapi/ip_tables.h"
We have to put netinet/in.h on the top to get rid of conflict of glibc and kernel headers for old unbuntu. ----------- /usr/include/linux/in.h:28:3: error: redeclaration of enumerator 'IPPROTO_IP' IPPROTO_IP = 0, /* Dummy protocol for TCP */ ^ /usr/include/netinet/in.h:42:5: note: previous definition of 'IPPROTO_IP' was here IPPROTO_IP = 0, /* Dummy protocol for TCP. */ ... ----------- See: https://www.mail-archive.com/netdev@vger.kernel.org/msg132711.html Fixes: ebf3a4fbd39a (Add setsockopt08, CVE-2021-22555) Signed-off-by: Li Wang <liwang@redhat.com> --- testcases/kernel/syscalls/setsockopt/setsockopt08.c | 2 ++ 1 file changed, 2 insertions(+)