[PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Ondrej Mosnacek]

This series aims to improve cipso_v4_skbuff_delattr() to fully
remove the CIPSO options instead of just clearing them with NOPs.
That is implemented in the second patch, while the first patch is
a bugfix for cipso_v4_delopt() that the second patch depends on.

Tested using selinux-testsuite a TMT/Beakerlib test from this PR:
https://src.fedoraproject.org/tests/selinux/pull-request/488

Changes in v2:
- drop the paranoid WARN_ON() usage
- reword the description of the second patch

v1: https://lore.kernel.org/linux-security-module/20240416152913.1527166-1-omosnace@redhat.com/

Ondrej Mosnacek (2):
  cipso: fix total option length computation
  cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

 net/ipv4/cipso_ipv4.c | 75 +++++++++++++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 21 deletions(-)

-- 
2.45.1

[PATCH v2 1/2] cipso: fix total option length computation

[Ondrej Mosnacek]

As evident from the definition of ip_options_get(), the IP option
IPOPT_END is used to pad the IP option data array, not IPOPT_NOP. Yet
the loop that walks the IP options to determine the total IP options
length in cipso_v4_delopt() doesn't take IPOPT_END into account.

Fix it by recognizing the IPOPT_END value as the end of actual options.

Fixes: 014ab19a69c3 ("selinux: Set socket NetLabel based on connection endpoint")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 net/ipv4/cipso_ipv4.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index dd6d460150580..5e9ac68444f89 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -2013,12 +2013,16 @@ static int cipso_v4_delopt(struct ip_options_rcu __rcu **opt_ptr)
 		 * from there we can determine the new total option length */
 		iter = 0;
 		optlen_new = 0;
-		while (iter < opt->opt.optlen)
-			if (opt->opt.__data[iter] != IPOPT_NOP) {
+		while (iter < opt->opt.optlen) {
+			if (opt->opt.__data[iter] == IPOPT_END) {
+				break;
+			} else if (opt->opt.__data[iter] == IPOPT_NOP) {
+				iter++;
+			} else {
 				iter += opt->opt.__data[iter + 1];
 				optlen_new = iter;
-			} else
-				iter++;
+			}
+		}
 		hdr_delta = opt->opt.optlen;
 		opt->opt.optlen = (optlen_new + 3) & ~3;
 		hdr_delta -= opt->opt.optlen;
-- 
2.45.1

[PATCH v2 2/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Ondrej Mosnacek]

As the comment in this function says, the code currently just clears the
CIPSO part with IPOPT_NOP, rather than removing it completely and
trimming the packet. The other cipso_v4_*_delattr() functions, however,
do the proper removal and also calipso_skbuff_delattr() makes an effort
to remove the CALIPSO options instead of replacing them with padding.

Some routers treat IPv4 packets with anything (even NOPs) in the option
header as a special case and take them through a slower processing path.
Consequently, hardening guides such as STIG recommend to configure such
routers to drop packets with non-empty IP option headers [1][2]. Thus,
users might expect NetLabel to produce packets with minimal padding (or
at least with no padding when no actual options are present).

Implement the proper option removal to address this and to be closer to
what the peer functions do.

[1] https://www.stigviewer.com/stig/juniper_router_rtr/2019-09-27/finding/V-90937
[2] https://www.stigviewer.com/stig/cisco_ios_xe_router_rtr/2021-03-26/finding/V-217001

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 net/ipv4/cipso_ipv4.c | 79 +++++++++++++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 25 deletions(-)

diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 5e9ac68444f89..e9cb27061c12e 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -1810,6 +1810,29 @@ static int cipso_v4_genopt(unsigned char *buf, u32 buf_len,
 	return CIPSO_V4_HDR_LEN + ret_val;
 }
 
+static int cipso_v4_get_actual_opt_len(const unsigned char *data, int len)
+{
+	int iter = 0, optlen = 0;
+
+	/* determining the new total option length is tricky because of
+	 * the padding necessary, the only thing i can think to do at
+	 * this point is walk the options one-by-one, skipping the
+	 * padding at the end to determine the actual option size and
+	 * from there we can determine the new total option length
+	 */
+	while (iter < len) {
+		if (data[iter] == IPOPT_END) {
+			break;
+		} else if (data[iter] == IPOPT_NOP) {
+			iter++;
+		} else {
+			iter += data[iter + 1];
+			optlen = iter;
+		}
+	}
+	return optlen;
+}
+
 /**
  * cipso_v4_sock_setattr - Add a CIPSO option to a socket
  * @sk: the socket
@@ -1986,7 +2009,6 @@ static int cipso_v4_delopt(struct ip_options_rcu __rcu **opt_ptr)
 		u8 cipso_len;
 		u8 cipso_off;
 		unsigned char *cipso_ptr;
-		int iter;
 		int optlen_new;
 
 		cipso_off = opt->opt.cipso - sizeof(struct iphdr);
@@ -2006,23 +2028,8 @@ static int cipso_v4_delopt(struct ip_options_rcu __rcu **opt_ptr)
 		memmove(cipso_ptr, cipso_ptr + cipso_len,
 			opt->opt.optlen - cipso_off - cipso_len);
 
-		/* determining the new total option length is tricky because of
-		 * the padding necessary, the only thing i can think to do at
-		 * this point is walk the options one-by-one, skipping the
-		 * padding at the end to determine the actual option size and
-		 * from there we can determine the new total option length */
-		iter = 0;
-		optlen_new = 0;
-		while (iter < opt->opt.optlen) {
-			if (opt->opt.__data[iter] == IPOPT_END) {
-				break;
-			} else if (opt->opt.__data[iter] == IPOPT_NOP) {
-				iter++;
-			} else {
-				iter += opt->opt.__data[iter + 1];
-				optlen_new = iter;
-			}
-		}
+		optlen_new = cipso_v4_get_actual_opt_len(opt->opt.__data,
+							 opt->opt.optlen);
 		hdr_delta = opt->opt.optlen;
 		opt->opt.optlen = (optlen_new + 3) & ~3;
 		hdr_delta -= opt->opt.optlen;
@@ -2242,7 +2249,8 @@ int cipso_v4_skbuff_setattr(struct sk_buff *skb,
  */
 int cipso_v4_skbuff_delattr(struct sk_buff *skb)
 {
-	int ret_val;
+	int ret_val, cipso_len, hdr_len_actual, new_hdr_len_actual, new_hdr_len,
+	    hdr_len_delta;
 	struct iphdr *iph;
 	struct ip_options *opt = &IPCB(skb)->opt;
 	unsigned char *cipso_ptr;
@@ -2255,16 +2263,37 @@ int cipso_v4_skbuff_delattr(struct sk_buff *skb)
 	if (ret_val < 0)
 		return ret_val;
 
-	/* the easiest thing to do is just replace the cipso option with noop
-	 * options since we don't change the size of the packet, although we
-	 * still need to recalculate the checksum */
-
 	iph = ip_hdr(skb);
 	cipso_ptr = (unsigned char *)iph + opt->cipso;
-	memset(cipso_ptr, IPOPT_NOOP, cipso_ptr[1]);
+	cipso_len = cipso_ptr[1];
+
+	hdr_len_actual = sizeof(struct iphdr) +
+			 cipso_v4_get_actual_opt_len((unsigned char *)(iph + 1),
+						     opt->optlen);
+	new_hdr_len_actual = hdr_len_actual - cipso_len;
+	new_hdr_len = (new_hdr_len_actual + 3) & ~3;
+	hdr_len_delta = (iph->ihl << 2) - new_hdr_len;
+
+	/* 1. shift any options after CIPSO to the left */
+	memmove(cipso_ptr, cipso_ptr + cipso_len,
+		new_hdr_len_actual - opt->cipso);
+	/* 2. move the whole IP header to its new place */
+	memmove((unsigned char *)iph + hdr_len_delta, iph, new_hdr_len_actual);
+	/* 3. adjust the skb layout */
+	skb_pull(skb, hdr_len_delta);
+	skb_reset_network_header(skb);
+	iph = ip_hdr(skb);
+	/* 4. re-fill new padding with IPOPT_END (may now be longer) */
+	memset((unsigned char *)iph + new_hdr_len_actual, IPOPT_END,
+	       new_hdr_len - new_hdr_len_actual);
+
+	opt->optlen -= hdr_len_delta;
 	opt->cipso = 0;
 	opt->is_changed = 1;
-
+	if (hdr_len_delta != 0) {
+		iph->ihl = new_hdr_len >> 2;
+		iph_set_totlen(iph, skb->len);
+	}
 	ip_send_check(iph);
 
 	return 0;
-- 
2.45.1

Re: [PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Casey Schaufler]

On 6/7/2024 9:07 AM, Ondrej Mosnacek wrote:
> This series aims to improve cipso_v4_skbuff_delattr() to fully
> remove the CIPSO options instead of just clearing them with NOPs.
> That is implemented in the second patch, while the first patch is
> a bugfix for cipso_v4_delopt() that the second patch depends on.
>
> Tested using selinux-testsuite a TMT/Beakerlib test from this PR:
> https://src.fedoraproject.org/tests/selinux/pull-request/488

Smack also uses CIPSO. The Smack testsuite is:
https://github.com/smack-team/smack-testsuite.git

>
> Changes in v2:
> - drop the paranoid WARN_ON() usage
> - reword the description of the second patch
>
> v1: https://lore.kernel.org/linux-security-module/20240416152913.1527166-1-omosnace@redhat.com/
>
> Ondrej Mosnacek (2):
>   cipso: fix total option length computation
>   cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options
>
>  net/ipv4/cipso_ipv4.c | 75 +++++++++++++++++++++++++++++++------------
>  1 file changed, 54 insertions(+), 21 deletions(-)
>

Re: [PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Ondrej Mosnacek]

On Fri, Jun 7, 2024 at 8:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 6/7/2024 9:07 AM, Ondrej Mosnacek wrote:
> > This series aims to improve cipso_v4_skbuff_delattr() to fully
> > remove the CIPSO options instead of just clearing them with NOPs.
> > That is implemented in the second patch, while the first patch is
> > a bugfix for cipso_v4_delopt() that the second patch depends on.
> >
> > Tested using selinux-testsuite a TMT/Beakerlib test from this PR:
> > https://src.fedoraproject.org/tests/selinux/pull-request/488
>
> Smack also uses CIPSO. The Smack testsuite is:
> https://github.com/smack-team/smack-testsuite.git

I tried to run it now, but 6 out of 114 tests fail for me already on
the baseline kernel (I tried with the v6.9 tag from mainline). The
output is not very verbose, so I'm not sure what is actually failing
and if it's caused by something on my side... With my patches applied,
the number of failed tests was the same, though, so there is no
evidence of a regression, at least.

-- 
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Casey Schaufler]

On 6/10/2024 8:14 AM, Ondrej Mosnacek wrote:
> On Fri, Jun 7, 2024 at 8:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 6/7/2024 9:07 AM, Ondrej Mosnacek wrote:
>>> This series aims to improve cipso_v4_skbuff_delattr() to fully
>>> remove the CIPSO options instead of just clearing them with NOPs.
>>> That is implemented in the second patch, while the first patch is
>>> a bugfix for cipso_v4_delopt() that the second patch depends on.
>>>
>>> Tested using selinux-testsuite a TMT/Beakerlib test from this PR:
>>> https://src.fedoraproject.org/tests/selinux/pull-request/488
>> Smack also uses CIPSO. The Smack testsuite is:
>> https://github.com/smack-team/smack-testsuite.git
> I tried to run it now, but 6 out of 114 tests fail for me already on
> the baseline kernel (I tried with the v6.9 tag from mainline). The
> output is not very verbose, so I'm not sure what is actually failing
> and if it's caused by something on my side... With my patches applied,
> the number of failed tests was the same, though, so there is no
> evidence of a regression, at least.

I assume you didn't select CONFIG_SECURITY_SMACK_NETFILTER, which
impacts some of the IPv6 test case. Thank you for running the tests.

>

Re: [PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Ondrej Mosnacek]

On Mon, Jun 10, 2024 at 6:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 6/10/2024 8:14 AM, Ondrej Mosnacek wrote:
> > On Fri, Jun 7, 2024 at 8:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 6/7/2024 9:07 AM, Ondrej Mosnacek wrote:
> >>> This series aims to improve cipso_v4_skbuff_delattr() to fully
> >>> remove the CIPSO options instead of just clearing them with NOPs.
> >>> That is implemented in the second patch, while the first patch is
> >>> a bugfix for cipso_v4_delopt() that the second patch depends on.
> >>>
> >>> Tested using selinux-testsuite a TMT/Beakerlib test from this PR:
> >>> https://src.fedoraproject.org/tests/selinux/pull-request/488
> >> Smack also uses CIPSO. The Smack testsuite is:
> >> https://github.com/smack-team/smack-testsuite.git
> > I tried to run it now, but 6 out of 114 tests fail for me already on
> > the baseline kernel (I tried with the v6.9 tag from mainline). The
> > output is not very verbose, so I'm not sure what is actually failing
> > and if it's caused by something on my side... With my patches applied,
> > the number of failed tests was the same, though, so there is no
> > evidence of a regression, at least.
>
> I assume you didn't select CONFIG_SECURITY_SMACK_NETFILTER, which
> impacts some of the IPv6 test case. Thank you for running the tests.

You're right, I only enabled SECURITY_SMACK and didn't look at the
other options. Enabling SECURITY_SMACK_NETFILTER fixed most of the
failures, but the audit-avc test is still failing:

./tests/audit-avc.sh:62 FAIL
./tests/audit-avc.sh:78 PASS
./tests/audit-avc.sh PASS=1 FAIL=1

I didn't try the baseline kernel this time, but looking at the test
script the failure doesn't appear to be related to the patches.

--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri,  7 Jun 2024 18:07:51 +0200 you wrote:
> This series aims to improve cipso_v4_skbuff_delattr() to fully
> remove the CIPSO options instead of just clearing them with NOPs.
> That is implemented in the second patch, while the first patch is
> a bugfix for cipso_v4_delopt() that the second patch depends on.
> 
> Tested using selinux-testsuite a TMT/Beakerlib test from this PR:
> https://src.fedoraproject.org/tests/selinux/pull-request/488
> 
> [...]

Here is the summary with links:
  - [v2,1/2] cipso: fix total option length computation
    https://git.kernel.org/netdev/net/c/9f3616991233
  - [v2,2/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri,  7 Jun 2024 18:07:51 +0200 you wrote:
> This series aims to improve cipso_v4_skbuff_delattr() to fully
> remove the CIPSO options instead of just clearing them with NOPs.
> That is implemented in the second patch, while the first patch is
> a bugfix for cipso_v4_delopt() that the second patch depends on.
> 
> Tested using selinux-testsuite a TMT/Beakerlib test from this PR:
> https://src.fedoraproject.org/tests/selinux/pull-request/488
> 
> [...]

Here is the summary with links:
  - [v2,1/2] cipso: fix total option length computation
    (no matching commit)
  - [v2,2/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options
    https://git.kernel.org/netdev/net/c/89aa3619d141

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Paul Moore]

On Fri, Jun 14, 2024 at 3:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>
> Hello:
>
> This series was applied to netdev/net.git (main)
> by David S. Miller <davem@davemloft.net>:

Welp, that was premature based on the testing requests in the other
thread, but what's done is done.

Ondrej, please accelerate the testing if possible as this patchset now
in the netdev tree and it would be good to know if it need a fix or
reverting before the next merge window.

> On Fri,  7 Jun 2024 18:07:51 +0200 you wrote:
> > This series aims to improve cipso_v4_skbuff_delattr() to fully
> > remove the CIPSO options instead of just clearing them with NOPs.
> > That is implemented in the second patch, while the first patch is
> > a bugfix for cipso_v4_delopt() that the second patch depends on.
> >
> > Tested using selinux-testsuite a TMT/Beakerlib test from this PR:
> > https://src.fedoraproject.org/tests/selinux/pull-request/488
> >
> > [...]
>
> Here is the summary with links:
>   - [v2,1/2] cipso: fix total option length computation
>     https://git.kernel.org/netdev/net/c/9f3616991233
>   - [v2,2/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options
>     (no matching commit)
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html

-- 
paul-moore.com

Re: [PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Paul Moore]

On June 14, 2024 11:08:41 AM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Jun 14, 2024 at 3:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>>
>> Hello:
>>
>> This series was applied to netdev/net.git (main)
>> by David S. Miller <davem@davemloft.net>:
>
> Welp, that was premature based on the testing requests in the other
> thread, but what's done is done.
>
> Ondrej, please accelerate the testing if possible as this patchset now
> in the netdev tree and it would be good to know if it need a fix or
> reverting before the next merge window.

Ondrej, can you confirm that you are currently working on testing this 
patchset as requested?

--
paul-moore.com

Re: [PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Ondrej Mosnacek]

On Wed, Jun 19, 2024 at 4:46 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On June 14, 2024 11:08:41 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Jun 14, 2024 at 3:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
> >>
> >> Hello:
> >>
> >> This series was applied to netdev/net.git (main)
> >> by David S. Miller <davem@davemloft.net>:
> >
> > Welp, that was premature based on the testing requests in the other
> > thread, but what's done is done.
> >
> > Ondrej, please accelerate the testing if possible as this patchset now
> > in the netdev tree and it would be good to know if it need a fix or
> > reverting before the next merge window.
>
> Ondrej, can you confirm that you are currently working on testing this
> patchset as requested?

Not really... I tried some more to get cloud-init to work on FreeBSD,
but still no luck... Anyway, I still don't see why I should waste my
time on testing this scenario, since you didn't provide any credible
hypothesis on why/what should break there. Convince me that there is a
valid concern and I will be much more willing to put more effort into
it. You see something there that I don't, and I'd like to see and
understand it, too. Let's turn it from *your* concern to *our* concern
(or lack of it) and then the cooperation will work better.

BTW, I was also surprised that David merged the patches quietly like
this. I don't know if he didn't see your comments or if he knowingly
dismissed them...

--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Paul Moore]

On Thu, Jun 20, 2024 at 6:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Jun 19, 2024 at 4:46 AM Paul Moore <paul@paul-moore.com> wrote:
> > On June 14, 2024 11:08:41 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Fri, Jun 14, 2024 at 3:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
> > >>
> > >> Hello:
> > >>
> > >> This series was applied to netdev/net.git (main)
> > >> by David S. Miller <davem@davemloft.net>:
> > >
> > > Welp, that was premature based on the testing requests in the other
> > > thread, but what's done is done.
> > >
> > > Ondrej, please accelerate the testing if possible as this patchset now
> > > in the netdev tree and it would be good to know if it need a fix or
> > > reverting before the next merge window.
> >
> > Ondrej, can you confirm that you are currently working on testing this
> > patchset as requested?

[NOTE: adding SELinux list as a FYI for potential breakage in upcoming kernels]

> Not really... I tried some more to get cloud-init to work on FreeBSD,
> but still no luck...

As mentioned previously, if you aren't able to fit the testing into
your automated framework, you'll need to do some manual testing to
verify the patches.

> Anyway, I still don't see why I should waste my
> time on testing this scenario, since you didn't provide any credible
> hypothesis on why/what should break there.

I did share my concern about changes in packet length across the
network path and an uncertainty about how different clients might
react.  While you tested with Linux based systems, I requested that
you test with at least one non-Linux client to help verify that things
are handled properly.

Perhaps you don't view that concern as credible, but it is something
I'm worried about as a common use case is for non-Linux clients to
connect over an unlabeled, single label/level network to a Linux
gateway which then routes traffic over different networks, some with
explicit labeling.

If you don't believe that testing this is important Ondrej, trust
those who have worked with a number of users who have deployed these
types of systems that this is important.

> Convince me that there is a
> valid concern and I will be much more willing to put more effort into
> it.

I've shared my concerns with you, both in previous threads and now in
this thread.  This really shouldn't be about convincing you to do The
Right Thing and verify that your patch doesn't break existing users,
it should be about you wanting to do The Right Thing so your work
doesn't break the kernel.

> You see something there that I don't, and I'd like to see and
> understand it, too. Let's turn it from *your* concern to *our* concern
> (or lack of it) and then the cooperation will work better.

It's not about you or I, it's about all of the users who rely on this
functionality and not wanting to break things for them.

Test your patches Ondrej, if you don't you'll find me increasingly
reluctant to accept anything from you in any of the trees I look
after.

> BTW, I was also surprised that David merged the patches quietly like
> this. I don't know if he didn't see your comments or if he knowingly
> dismissed them...

I've seen DaveM do this before, but as Jakub has been the one pulling
the labeled networking patches after my ACK I thought DaveM was no
longer doing that type of work.  My guess based on previous experience
is that DaveM didn't see any comments on your latest patchset - as we
were still discussing things in the previous thread - but only DaveM
can comment on that, and it really isn't very important at this point.

-- 
paul-moore.com

Re: [PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Ondrej Mosnacek]

On Thu, Jun 20, 2024 at 4:39 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Jun 20, 2024 at 6:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Wed, Jun 19, 2024 at 4:46 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On June 14, 2024 11:08:41 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Fri, Jun 14, 2024 at 3:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
> > > >>
> > > >> Hello:
> > > >>
> > > >> This series was applied to netdev/net.git (main)
> > > >> by David S. Miller <davem@davemloft.net>:
> > > >
> > > > Welp, that was premature based on the testing requests in the other
> > > > thread, but what's done is done.
> > > >
> > > > Ondrej, please accelerate the testing if possible as this patchset now
> > > > in the netdev tree and it would be good to know if it need a fix or
> > > > reverting before the next merge window.
> > >
> > > Ondrej, can you confirm that you are currently working on testing this
> > > patchset as requested?
>
> [NOTE: adding SELinux list as a FYI for potential breakage in upcoming kernels]
>
> > Not really... I tried some more to get cloud-init to work on FreeBSD,
> > but still no luck...
>
> As mentioned previously, if you aren't able to fit the testing into
> your automated framework, you'll need to do some manual testing to
> verify the patches.

Sigh... okay, I now did test the scenario with a FreeBSD system as B
and it passed.

> > Anyway, I still don't see why I should waste my
> > time on testing this scenario, since you didn't provide any credible
> > hypothesis on why/what should break there.
>
> I did share my concern about changes in packet length across the
> network path and an uncertainty about how different clients might
> react.  While you tested with Linux based systems, I requested that
> you test with at least one non-Linux client to help verify that things
> are handled properly.
>
> Perhaps you don't view that concern as credible, but it is something
> I'm worried about as a common use case is for non-Linux clients to
> connect over an unlabeled, single label/level network to a Linux
> gateway which then routes traffic over different networks, some with
> explicit labeling.
>
> If you don't believe that testing this is important Ondrej, trust
> those who have worked with a number of users who have deployed these
> types of systems that this is important.

I'm not saying the concern is not credible or that (in general)
testing this use case is not important. What I'm missing is some
explanation/reasoning that would make me think "Oh yeah, these patches
really could break this scenario". You said something about consistent
IP header overhead and bidirectional stream-based connections, but I
don't understand how the former could cause an issue with the latter.
Does it violate some specification? And I'm not arguing that there
isn't a possible bug because I don't see it; I'm just arguing that if
there is a mechanism through which the change could cause a bug in
this scenario, you should be able to explain it (at least roughly) to
someone who doesn't see it there.

>
> > Convince me that there is a
> > valid concern and I will be much more willing to put more effort into
> > it.
>
> I've shared my concerns with you, both in previous threads and now in
> this thread.  This really shouldn't be about convincing you to do The
> Right Thing and verify that your patch doesn't break existing users,
> it should be about you wanting to do The Right Thing so your work
> doesn't break the kernel.
>
> > You see something there that I don't, and I'd like to see and
> > understand it, too. Let's turn it from *your* concern to *our* concern
> > (or lack of it) and then the cooperation will work better.
>
> It's not about you or I, it's about all of the users who rely on this
> functionality and not wanting to break things for them.
>
> Test your patches Ondrej, if you don't you'll find me increasingly
> reluctant to accept anything from you in any of the trees I look
> after.

Paul, I don't want to break the kernel, but that doesn't mean I will
do an excessive amount of work for someone else when there doesn't
seem to be a logical reason to do so. IMHO, just because someone
somewhere has a special hard-to-test use case that is very important
to them doesn't mean that it is your job as a community project
maintainer to force other contributors to do work to defend these
peoples' use cases. The fact that we don't see any effort from them to
have their use case tested upstream (they could either auto-run their
tests on patches themselves and mail back the results or provide the
test code and/or infrastructure and ask maintainers + contributors to
run the tests on patches before they are merged) means that one of the
following is true:
1. They do care about upstream not breaking their use case, but expect
that upstream will automatically ensure that "for free".
2. They accept the fact that upstream may break their use case and
they rely on testing at a later stage to find issues and
reporting/fixing the bugs once they are found. Usually this is because
they have calculated / assume that at the given time, the cost of
implementing/facilitating testing on upstream level is higher than the
cost of finding the bugs later and waiting for the upstream kernel to
become usable again.
In both cases it doesn't make sense for the community to self-impose
the need to substitute the lack of effort from the consumer side, and
much less so to push that effort to others (which will just drive
contributors away, which is a far worse consequence than possibly
breaking some complex scenario once in a while). However, as soon as
someone comes and says "Hey, we have this test, this is how you can
run it. Can you make sure patches touching X are tested with it? If
it's too much of a hassle, let us help to make it work.", it's an
entirely different situation and we (both contributors and
maintainers) should very much do our best to fulfill the request.

This is my idea of how the fine balance in the
user-maintainer-contributor relationship could work best. I'm writing
it here so that you can understand where I'm coming from and perhaps
ponder about it a bit. But the main point here is that you requested a
complicated scenario to be tested without adequately explaining what
you assume to be possibly broken. And I also pointed out that the
behavior you seem to think can cause breakage is already present in
the current code - you didn't answer that. So I feel like you
arbitrarily push some high-effort requirement on me and that's not
nice.

--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH v2 0/2] cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options

[Paul Moore]

On Fri, Jul 26, 2024 at 8:44 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Jun 20, 2024 at 4:39 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Jun 20, 2024 at 6:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Wed, Jun 19, 2024 at 4:46 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On June 14, 2024 11:08:41 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Fri, Jun 14, 2024 at 3:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
> > > > >>
> > > > >> Hello:
> > > > >>
> > > > >> This series was applied to netdev/net.git (main)
> > > > >> by David S. Miller <davem@davemloft.net>:
> > > > >
> > > > > Welp, that was premature based on the testing requests in the other
> > > > > thread, but what's done is done.
> > > > >
> > > > > Ondrej, please accelerate the testing if possible as this patchset now
> > > > > in the netdev tree and it would be good to know if it need a fix or
> > > > > reverting before the next merge window.
> > > >
> > > > Ondrej, can you confirm that you are currently working on testing this
> > > > patchset as requested?
> >
> > [NOTE: adding SELinux list as a FYI for potential breakage in upcoming kernels]
> >
> > > Not really... I tried some more to get cloud-init to work on FreeBSD,
> > > but still no luck...
> >
> > As mentioned previously, if you aren't able to fit the testing into
> > your automated framework, you'll need to do some manual testing to
> > verify the patches.
>
> Sigh... okay, I now did test the scenario with a FreeBSD system as B
> and it passed.

Great, thank you.

> I'm not saying the concern is not credible or that (in general)
> testing this use case is not important. What I'm missing is some
> explanation/reasoning that would make me think "Oh yeah, these patches
> really could break this scenario" ...

One of the challenges to network testing is that you don't always know
how other network stack implementations are going to react when you
start getting into corner cases or lesser implemented protocols.  You
just need to test your patches to make sure nothing breaks.

> > > You see something there that I don't, and I'd like to see and
> > > understand it, too. Let's turn it from *your* concern to *our* concern
> > > (or lack of it) and then the cooperation will work better.
> >
> > It's not about you or I, it's about all of the users who rely on this
> > functionality and not wanting to break things for them.
> >
> > Test your patches Ondrej, if you don't you'll find me increasingly
> > reluctant to accept anything from you in any of the trees I look
> > after.
>
> Paul, I don't want to break the kernel, but that doesn't mean I will
> do an excessive amount of work for someone else when there doesn't
> seem to be a logical reason to do so. IMHO, just because someone
> somewhere has a special hard-to-test use case that is very important
> to them doesn't mean that it is your job as a community project
> maintainer to force other contributors to do work to defend these
> peoples' use cases.

I have a responsibility to ensure that we provide a stable, secure,
maintainable kernel that is as bug-free as we can possibly make it.
If I see a patch that I believe warrants a certain type of test to
help meet those goals I'm going to ask for that testing.  Of course
like many things, even things we believe to be very clear, there is
always going to be a chance that disagreements will happen around what
testing is relevant or necessary.  How you handle that disagreement is
a choice you will need to make for yourself, but I would encourage you
to consider that more testing is usually a good thing, and aggravating
those who review/ACK your patches is generally not a good long term
strategy.

-- 
paul-moore.com