[LTP] [COMMITTED] setsockopt08: includes netinet/in.h

[LTP] [COMMITTED] setsockopt08: includes netinet/in.h

[Li Wang]

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

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"
-- 
2.31.1

[LTP] [COMMITTED] setsockopt08: includes netinet/in.h

[Petr Vorel]

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"

[LTP] [COMMITTED] setsockopt08: includes netinet/in.h

[Petr Vorel]

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"

[LTP] [COMMITTED] setsockopt08: includes netinet/in.h

[Li Wang]

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"

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210806/7d3c06f4/attachment.htm>

[LTP] [COMMITTED] setsockopt08: includes netinet/in.h

[Petr Vorel]

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"

[LTP] [COMMITTED] setsockopt08: includes netinet/in.h

[Li Wang]

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

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210806/eb7401a6/attachment.htm>

[LTP] [COMMITTED] setsockopt08: includes netinet/in.h

[Richard Palethorpe]

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.

-- 
Thank you,
Richard.

[LTP] [COMMITTED] setsockopt08: includes netinet/in.h

[Li Wang]

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


-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210809/12a4a456/attachment.htm>

[LTP] [COMMITTED] setsockopt08: includes netinet/in.h

[Petr Vorel]

> 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