| Message ID | 20240607160753.1787105-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options | expand |
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(-) >
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.
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. >
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.
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!
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!
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
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
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.
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.
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.
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.