* Re: [PATCH v2] dt-bindings: net: orion-mdio: Convert to JSON schema
From: Chris Packham @ 2022-05-16 22:18 UTC (permalink / raw)
To: Rob Herring
Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, krzysztof.kozlowski+dt@linaro.org,
andrew@lunn.ch, netdev@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Marek Behún
In-Reply-To: <20220516181639.GB2997786-robh@kernel.org>
Hi Rob,
On 17/05/22 06:16, Rob Herring wrote:
> On Fri, May 06, 2022 at 09:06:20AM +1200, Chris Packham wrote:
>> Convert the marvell,orion-mdio binding to JSON schema.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>> This does throw up the following dtbs_check warnings for turris-mox:
>>
>> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: mdio@32004: switch0@10:reg: [[16], [0]] is too long
>> From schema: Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: mdio@32004: switch0@2:reg: [[2], [0]] is too long
>> From schema: Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: mdio@32004: switch1@11:reg: [[17], [0]] is too long
>> From schema: Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: mdio@32004: switch1@2:reg: [[2], [0]] is too long
>> From schema: Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: mdio@32004: switch2@12:reg: [[18], [0]] is too long
>> From schema: Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: mdio@32004: switch2@2:reg: [[2], [0]] is too long
>> From schema: Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>>
>> I think they're all genuine but I'm hesitant to leap in and fix them
>> without being able to test them.
>>
>> I also need to set unevaluatedProperties: true to cater for the L2
>> switch on turris-mox (and probably others). That might be better tackled
>> in the core mdio.yaml schema but I wasn't planning on touching that.
>>
>> Changes in v2:
>> - Add Andrew as maintainer (thanks for volunteering)
>>
>> .../bindings/net/marvell,orion-mdio.yaml | 60 +++++++++++++++++++
>> .../bindings/net/marvell-orion-mdio.txt | 54 -----------------
>> 2 files changed, 60 insertions(+), 54 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>> delete mode 100644 Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml b/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>> new file mode 100644
>> index 000000000000..fe3a3412f093
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>> @@ -0,0 +1,60 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://scanmail.trustwave.com/?c=20988&d=j5WC4r_pZnDMILajH1kaKLL9oC7kQjgv_bkDWJOhEQ&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fnet%2fmarvell%2corion-mdio%2eyaml%23
>> +$schema: http://scanmail.trustwave.com/?c=20988&d=j5WC4r_pZnDMILajH1kaKLL9oC7kQjgv_e4CDcetEw&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>> +
>> +title: Marvell MDIO Ethernet Controller interface
>> +
>> +maintainers:
>> + - Andrew Lunn <andrew@lunn.ch>
>> +
>> +description: |
>> + The Ethernet controllers of the Marvel Kirkwood, Dove, Orion5x, MV78xx0,
>> + Armada 370, Armada XP, Armada 7k and Armada 8k have an identical unit that
>> + provides an interface with the MDIO bus. Additionally, Armada 7k and Armada
>> + 8k has a second unit which provides an interface with the xMDIO bus. This
>> + driver handles these interfaces.
>> +
>> +allOf:
>> + - $ref: "mdio.yaml#"
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - marvell,orion-mdio
>> + - marvell,xmdio
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + clocks:
>> + minItems: 1
>> + maxItems: 4
> Really this should be better defined, but the original was not.
>
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +unevaluatedProperties: true
> This must be false.
Right now there is no way (that I have found) of dealing with non-PHY
devices like the dsa switches so setting this to false generates
warnings on turris-mox:
arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: mdio@32004:
Unevaluated properties are not allowed ('#address-cells', '#size-cells',
'ethernet-phy@1', 'switch0@10', 'switch0@2', 'switch1@11', 'switch1@2',
'switch2@12', 'switch2@2' were unexpected)
From schema:
/home/chrisp/src/linux/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
There are also warnings about the size of the reg property but these
seem to be genuine problems that Marek is looking at.
This change has already been picked up for net-next but if you have
suggestions for improvement I'm happy to submit them as additional
changes on-top of that.
>
>> +
>> +examples:
>> + - |
>> + mdio@d0072004 {
>> + compatible = "marvell,orion-mdio";
>> + reg = <0xd0072004 0x4>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + interrupts = <30>;
>> +
>> + phy0: ethernet-phy@0 {
>> + reg = <0>;
>> + };
>> +
>> + phy1: ethernet-phy@1 {
>> + reg = <1>;
>> + };
>> + };
>> diff --git a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt b/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
>> deleted file mode 100644
>> index 3f3cfc1d8d4d..000000000000
>> --- a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
>> +++ /dev/null
>> @@ -1,54 +0,0 @@
>> -* Marvell MDIO Ethernet Controller interface
>> -
>> -The Ethernet controllers of the Marvel Kirkwood, Dove, Orion5x,
>> -MV78xx0, Armada 370, Armada XP, Armada 7k and Armada 8k have an
>> -identical unit that provides an interface with the MDIO bus.
>> -Additionally, Armada 7k and Armada 8k has a second unit which
>> -provides an interface with the xMDIO bus. This driver handles
>> -these interfaces.
>> -
>> -Required properties:
>> -- compatible: "marvell,orion-mdio" or "marvell,xmdio"
>> -- reg: address and length of the MDIO registers. When an interrupt is
>> - not present, the length is the size of the SMI register (4 bytes)
>> - otherwise it must be 0x84 bytes to cover the interrupt control
>> - registers.
>> -
>> -Optional properties:
>> -- interrupts: interrupt line number for the SMI error/done interrupt
>> -- clocks: phandle for up to four required clocks for the MDIO instance
>> -
>> -The child nodes of the MDIO driver are the individual PHY devices
>> -connected to this MDIO bus. They must have a "reg" property given the
>> -PHY address on the MDIO bus.
>> -
>> -Example at the SoC level without an interrupt property:
>> -
>> -mdio {
>> - #address-cells = <1>;
>> - #size-cells = <0>;
>> - compatible = "marvell,orion-mdio";
>> - reg = <0xd0072004 0x4>;
>> -};
>> -
>> -Example with an interrupt property:
>> -
>> -mdio {
>> - #address-cells = <1>;
>> - #size-cells = <0>;
>> - compatible = "marvell,orion-mdio";
>> - reg = <0xd0072004 0x84>;
>> - interrupts = <30>;
>> -};
>> -
>> -And at the board level:
>> -
>> -mdio {
>> - phy0: ethernet-phy@0 {
>> - reg = <0>;
>> - };
>> -
>> - phy1: ethernet-phy@1 {
>> - reg = <1>;
>> - };
>> -}
>> --
>> 2.36.0
>>
>>
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: net: orion-mdio: Convert to JSON schema
From: Chris Packham @ 2022-05-16 22:27 UTC (permalink / raw)
To: Rob Herring
Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, krzysztof.kozlowski+dt@linaro.org,
andrew@lunn.ch, netdev@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Marek Behún
In-Reply-To: <0c67af76-df5e-1684-820c-f28aa6f50fe1@alliedtelesis.co.nz>
On 17/05/22 10:18, Chris Packham wrote:
> Hi Rob,
>
> On 17/05/22 06:16, Rob Herring wrote:
>> On Fri, May 06, 2022 at 09:06:20AM +1200, Chris Packham wrote:
>>> Convert the marvell,orion-mdio binding to JSON schema.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> ---
>>>
>>> Notes:
>>> This does throw up the following dtbs_check warnings for
>>> turris-mox:
>>> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: mdio@32004:
>>> switch0@10:reg: [[16], [0]] is too long
>>> From schema:
>>> Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>>> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb:
>>> mdio@32004: switch0@2:reg: [[2], [0]] is too long
>>> From schema:
>>> Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>>> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb:
>>> mdio@32004: switch1@11:reg: [[17], [0]] is too long
>>> From schema:
>>> Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>>> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb:
>>> mdio@32004: switch1@2:reg: [[2], [0]] is too long
>>> From schema:
>>> Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>>> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb:
>>> mdio@32004: switch2@12:reg: [[18], [0]] is too long
>>> From schema:
>>> Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>>> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb:
>>> mdio@32004: switch2@2:reg: [[2], [0]] is too long
>>> From schema:
>>> Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>>> I think they're all genuine but I'm hesitant to leap in
>>> and fix them
>>> without being able to test them.
>>> I also need to set unevaluatedProperties: true to cater
>>> for the L2
>>> switch on turris-mox (and probably others). That might be
>>> better tackled
>>> in the core mdio.yaml schema but I wasn't planning on touching
>>> that.
>>> Changes in v2:
>>> - Add Andrew as maintainer (thanks for volunteering)
>>>
>>> .../bindings/net/marvell,orion-mdio.yaml | 60
>>> +++++++++++++++++++
>>> .../bindings/net/marvell-orion-mdio.txt | 54 -----------------
>>> 2 files changed, 60 insertions(+), 54 deletions(-)
>>> create mode 100644
>>> Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>>> delete mode 100644
>>> Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>>> b/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>>> new file mode 100644
>>> index 000000000000..fe3a3412f093
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>>> @@ -0,0 +1,60 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id:
>>> http://scanmail.trustwave.com/?c=20988&d=j5WC4r_pZnDMILajH1kaKLL9oC7kQjgv_bkDWJOhEQ&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fnet%2fmarvell%2corion-mdio%2eyaml%23
>>> +$schema:
>>> http://scanmail.trustwave.com/?c=20988&d=j5WC4r_pZnDMILajH1kaKLL9oC7kQjgv_e4CDcetEw&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>>> +
>>> +title: Marvell MDIO Ethernet Controller interface
>>> +
>>> +maintainers:
>>> + - Andrew Lunn <andrew@lunn.ch>
>>> +
>>> +description: |
>>> + The Ethernet controllers of the Marvel Kirkwood, Dove, Orion5x,
>>> MV78xx0,
>>> + Armada 370, Armada XP, Armada 7k and Armada 8k have an identical
>>> unit that
>>> + provides an interface with the MDIO bus. Additionally, Armada 7k
>>> and Armada
>>> + 8k has a second unit which provides an interface with the xMDIO
>>> bus. This
>>> + driver handles these interfaces.
>>> +
>>> +allOf:
>>> + - $ref: "mdio.yaml#"
>>> +
>>> +properties:
>>> + compatible:
>>> + enum:
>>> + - marvell,orion-mdio
>>> + - marvell,xmdio
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + interrupts:
>>> + maxItems: 1
>>> +
>>> + clocks:
>>> + minItems: 1
>>> + maxItems: 4
>> Really this should be better defined, but the original was not.
>>
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> +
>>> +unevaluatedProperties: true
>> This must be false.
>
> Right now there is no way (that I have found) of dealing with non-PHY
> devices like the dsa switches so setting this to false generates
> warnings on turris-mox:
>
> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: mdio@32004:
> Unevaluated properties are not allowed ('#address-cells',
> '#size-cells', 'ethernet-phy@1', 'switch0@10', 'switch0@2',
> 'switch1@11', 'switch1@2', 'switch2@12', 'switch2@2' were unexpected)
> From schema:
> /home/chrisp/src/linux/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
>
> There are also warnings about the size of the reg property but these
> seem to be genuine problems that Marek is looking at.
Actually it looks if I fix the reg problem the need for
unevaluatedProperties goes away. I'll whip up a small patch series on
top of net-next.
>
> This change has already been picked up for net-next but if you have
> suggestions for improvement I'm happy to submit them as additional
> changes on-top of that.
>
>>
>>> +
>>> +examples:
>>> + - |
>>> + mdio@d0072004 {
>>> + compatible = "marvell,orion-mdio";
>>> + reg = <0xd0072004 0x4>;
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + interrupts = <30>;
>>> +
>>> + phy0: ethernet-phy@0 {
>>> + reg = <0>;
>>> + };
>>> +
>>> + phy1: ethernet-phy@1 {
>>> + reg = <1>;
>>> + };
>>> + };
>>> diff --git
>>> a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
>>> b/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
>>> deleted file mode 100644
>>> index 3f3cfc1d8d4d..000000000000
>>> --- a/Documentation/devicetree/bindings/net/marvell-orion-mdio.txt
>>> +++ /dev/null
>>> @@ -1,54 +0,0 @@
>>> -* Marvell MDIO Ethernet Controller interface
>>> -
>>> -The Ethernet controllers of the Marvel Kirkwood, Dove, Orion5x,
>>> -MV78xx0, Armada 370, Armada XP, Armada 7k and Armada 8k have an
>>> -identical unit that provides an interface with the MDIO bus.
>>> -Additionally, Armada 7k and Armada 8k has a second unit which
>>> -provides an interface with the xMDIO bus. This driver handles
>>> -these interfaces.
>>> -
>>> -Required properties:
>>> -- compatible: "marvell,orion-mdio" or "marvell,xmdio"
>>> -- reg: address and length of the MDIO registers. When an interrupt is
>>> - not present, the length is the size of the SMI register (4 bytes)
>>> - otherwise it must be 0x84 bytes to cover the interrupt control
>>> - registers.
>>> -
>>> -Optional properties:
>>> -- interrupts: interrupt line number for the SMI error/done interrupt
>>> -- clocks: phandle for up to four required clocks for the MDIO instance
>>> -
>>> -The child nodes of the MDIO driver are the individual PHY devices
>>> -connected to this MDIO bus. They must have a "reg" property given the
>>> -PHY address on the MDIO bus.
>>> -
>>> -Example at the SoC level without an interrupt property:
>>> -
>>> -mdio {
>>> - #address-cells = <1>;
>>> - #size-cells = <0>;
>>> - compatible = "marvell,orion-mdio";
>>> - reg = <0xd0072004 0x4>;
>>> -};
>>> -
>>> -Example with an interrupt property:
>>> -
>>> -mdio {
>>> - #address-cells = <1>;
>>> - #size-cells = <0>;
>>> - compatible = "marvell,orion-mdio";
>>> - reg = <0xd0072004 0x84>;
>>> - interrupts = <30>;
>>> -};
>>> -
>>> -And at the board level:
>>> -
>>> -mdio {
>>> - phy0: ethernet-phy@0 {
>>> - reg = <0>;
>>> - };
>>> -
>>> - phy1: ethernet-phy@1 {
>>> - reg = <1>;
>>> - };
>>> -}
>>> --
>>> 2.36.0
>>>
>>>
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: skb: Remove skb_data_area_size()
From: patchwork-bot+netdevbpf @ 2022-05-16 22:40 UTC (permalink / raw)
To: Martinez, Ricardo
Cc: netdev, linux-wireless, kuba, davem, johannes, ryazanov.s.a,
loic.poulain, m.chetan.kumar, chandrashekar.devegowda, linuxwwan,
chiranjeevi.rapolu, haijun.liu, andriy.shevchenko, dinesh.sharma,
ilpo.jarvinen, moises.veleta, sreehari.kancharla
In-Reply-To: <20220513173400.3848271-1-ricardo.martinez@linux.intel.com>
Hello:
This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Fri, 13 May 2022 10:33:58 -0700 you wrote:
> This patch series removes the skb_data_area_size() helper,
> replacing it in t7xx driver with the size used during skb allocation.
>
> https://lore.kernel.org/netdev/CAHNKnsTmH-rGgWi3jtyC=ktM1DW2W1VJkYoTMJV2Z_Bt498bsg@mail.gmail.com/
>
> Ricardo Martinez (2):
> net: wwan: t7xx: Avoid calls to skb_data_area_size()
> net: skb: Remove skb_data_area_size()
>
> [...]
Here is the summary with links:
- [net-next,1/2] net: wwan: t7xx: Avoid calls to skb_data_area_size()
https://git.kernel.org/netdev/net-next/c/262d98b1193f
- [net-next,2/2] net: skb: Remove skb_data_area_size()
https://git.kernel.org/netdev/net-next/c/89af2ce2d95c
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock
From: Jakub Kicinski @ 2022-05-16 22:40 UTC (permalink / raw)
To: Josua Mayer
Cc: Michael Walle, alexandru.ardelean, alvaro.karsz, davem, edumazet,
krzysztof.kozlowski+dt, michael.hennerich, netdev, pabeni,
robh+dt
In-Reply-To: <ab86220b-f583-4c77-0ddf-a3e25f5bc840@solid-run.com>
On Mon, 16 May 2022 22:48:20 +0300 Josua Mayer wrote:
> So I can imagine to change the bindings as follows:
> 1. remove the -recovered variants
> 2. add an explicit note in the commit message that the recovered clock
> is not implemented because we do not have infrastructure for SyncE
> 3. keep the -free-running suffix, we should imo only hide it on the day
> SyncE can be toggled by another means.
SGTM, thanks!
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/7] selftests/bpf: add MPTCP test base
From: Andrii Nakryiko @ 2022-05-16 22:43 UTC (permalink / raw)
To: Mat Martineau
Cc: Networking, bpf, Nicolas Rybowski, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, mptcp, Matthieu Baerts,
Geliang Tang
In-Reply-To: <20220513224827.662254-4-mathew.j.martineau@linux.intel.com>
On Fri, May 13, 2022 at 3:48 PM Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
>
> From: Nicolas Rybowski <nicolas.rybowski@tessares.net>
>
> This patch adds a base for MPTCP specific tests.
>
> It is currently limited to the is_mptcp field in case of plain TCP
> connection because there is no easy way to get the subflow sk from a msk
> in userspace. This implies that we cannot lookup the sk_storage attached
> to the subflow sk in the sockops program.
>
> v4:
> - add copyright 2022 (Andrii)
> - use ASSERT_* instead of CHECK_FAIL (Andrii)
> - drop SEC("version") (Andrii)
> - use is_mptcp in tcp_sock, instead of bpf_tcp_sock (Martin & Andrii)
>
> Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> Co-developed-by: Geliang Tang <geliang.tang@suse.com>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> Signed-off-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> ---
> MAINTAINERS | 1 +
> tools/testing/selftests/bpf/bpf_tcp_helpers.h | 1 +
> tools/testing/selftests/bpf/config | 1 +
> tools/testing/selftests/bpf/network_helpers.c | 43 ++++--
> tools/testing/selftests/bpf/network_helpers.h | 4 +
> .../testing/selftests/bpf/prog_tests/mptcp.c | 136 ++++++++++++++++++
> .../testing/selftests/bpf/progs/mptcp_sock.c | 53 +++++++
> 7 files changed, 231 insertions(+), 8 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/mptcp.c
> create mode 100644 tools/testing/selftests/bpf/progs/mptcp_sock.c
>
Seems like bpf_core_field_exists() works fine for your use case and CI
is green. See some selftest-specific issues below, though.
[...]
> +static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
> +{
> + int client_fd, prog_fd, map_fd, err;
> + struct bpf_program *prog;
> + struct bpf_object *obj;
> + struct bpf_map *map;
> +
> + obj = bpf_object__open("./mptcp_sock.o");
> + if (libbpf_get_error(obj))
> + return -EIO;
> +
> + err = bpf_object__load(obj);
> + if (!ASSERT_OK(err, "bpf_object__load"))
> + goto out;
> +
> + prog = bpf_object__find_program_by_name(obj, "_sockops");
can you please use BPF skeleton instead of doing these lookups by
name? See other tests that are including .skel.h headers for example
> + if (!ASSERT_OK_PTR(prog, "bpf_object__find_program_by_name")) {
> + err = -EIO;
> + goto out;
> + }
> +
[...]
> +void test_base(void)
> +{
> + int server_fd, cgroup_fd;
> +
> + cgroup_fd = test__join_cgroup("/mptcp");
> + if (CHECK_FAIL(cgroup_fd < 0))
> + return;
> +
> + /* without MPTCP */
> + server_fd = start_server(AF_INET, SOCK_STREAM, NULL, 0, 0);
> + if (CHECK_FAIL(server_fd < 0))
> + goto with_mptcp;
> +
> + CHECK_FAIL(run_test(cgroup_fd, server_fd, false));
please don't add new uses of CHECK_FAIL()
> +
> + close(server_fd);
> +
[...]
^ permalink raw reply
* Re: [PATCH bpf-next v4 5/7] selftests/bpf: verify token of struct mptcp_sock
From: Andrii Nakryiko @ 2022-05-16 22:45 UTC (permalink / raw)
To: Mat Martineau
Cc: Networking, bpf, Geliang Tang, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, mptcp, Matthieu Baerts
In-Reply-To: <20220513224827.662254-6-mathew.j.martineau@linux.intel.com>
On Fri, May 13, 2022 at 3:48 PM Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
>
> From: Geliang Tang <geliang.tang@suse.com>
>
> This patch verifies the struct member token of struct mptcp_sock. Add a
> new function get_msk_token() to parse the msk token from the output of
> the command 'ip mptcp monitor', and verify it in verify_msk().
>
> v4:
> - use ASSERT_* instead of CHECK_FAIL (Andrii)
> - skip the test if 'ip mptcp monitor' is not supported (Mat)
>
> Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> ---
> tools/testing/selftests/bpf/bpf_tcp_helpers.h | 1 +
> .../testing/selftests/bpf/prog_tests/mptcp.c | 64 +++++++++++++++++++
> .../testing/selftests/bpf/progs/mptcp_sock.c | 5 ++
> 3 files changed, 70 insertions(+)
>
[...]
> + fd = open(monitor_log_path, O_RDONLY);
> + if (!ASSERT_GE(fd, 0, "Failed to open monitor_log_path"))
> + return token;
> +
> + len = read(fd, buf, sizeof(buf));
> + if (!ASSERT_GT(len, 0, "Failed to read monitor_log_path"))
> + goto err;
> +
> + if (strncmp(buf, prefix, strlen(prefix))) {
ASSERT_STRNEQ ?
> + log_err("Invalid prefix %s", buf);
> + goto err;
> + }
> +
> + token = strtol(buf + strlen(prefix), NULL, 16);
> +
> +err:
> + close(fd);
> + return token;
> +}
> +
> static int verify_msk(int map_fd, int client_fd)
> {
> char *msg = "MPTCP subflow socket";
> int err, cfd = client_fd;
> struct mptcp_storage val;
> + __u32 token;
> +
> + token = get_msk_token();
> + if (!ASSERT_GT(token, 0, "Unexpected token"))
> + return -1;
>
> err = bpf_map_lookup_elem(map_fd, &cfd, &val);
> if (!ASSERT_OK(err, "bpf_map_lookup_elem"))
> @@ -58,6 +102,12 @@ static int verify_msk(int map_fd, int client_fd)
> err++;
> }
>
> + if (val.token != token) {
ASSERT_NEQ
> + log_err("Unexpected mptcp_sock.token %x != %x",
> + val.token, token);
> + err++;
> + }
> +
> return err;
> }
>
[...]
^ permalink raw reply
* Re: [PATCH bpf-next v4 4/7] selftests/bpf: test bpf_skc_to_mptcp_sock
From: Andrii Nakryiko @ 2022-05-16 22:47 UTC (permalink / raw)
To: Mat Martineau
Cc: Networking, bpf, Geliang Tang, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, mptcp, Matthieu Baerts
In-Reply-To: <20220513224827.662254-5-mathew.j.martineau@linux.intel.com>
On Fri, May 13, 2022 at 3:48 PM Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
>
> From: Geliang Tang <geliang.tang@suse.com>
>
> This patch extends the MPTCP test base, to test the new helper
> bpf_skc_to_mptcp_sock().
>
> Define struct mptcp_sock in bpf_tcp_helpers.h, use bpf_skc_to_mptcp_sock
> to get the msk socket in progs/mptcp_sock.c and store the infos in
> socket_storage_map.
>
> Get the infos from socket_storage_map in prog_tests/mptcp.c. Add a new
> function verify_msk() to verify the infos of MPTCP socket, and rename
> verify_sk() to verify_tsk() to verify TCP socket only.
>
> v2: Add CONFIG_MPTCP check for clearer error messages
> v4:
> - use ASSERT_* instead of CHECK_FAIL (Andrii)
> - drop bpf_mptcp_helpers.h (Andrii)
>
> Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> ---
> tools/testing/selftests/bpf/bpf_tcp_helpers.h | 5 +++
> .../testing/selftests/bpf/prog_tests/mptcp.c | 45 ++++++++++++++-----
> .../testing/selftests/bpf/progs/mptcp_sock.c | 23 ++++++++--
> 3 files changed, 58 insertions(+), 15 deletions(-)
>
[...]
> +static int verify_msk(int map_fd, int client_fd)
> +{
> + char *msg = "MPTCP subflow socket";
> + int err, cfd = client_fd;
> + struct mptcp_storage val;
> +
> + err = bpf_map_lookup_elem(map_fd, &cfd, &val);
> + if (!ASSERT_OK(err, "bpf_map_lookup_elem"))
> + return err;
> +
> + if (val.invoked != 1) {
> + log_err("%s: unexpected invoked count %d != 1",
> + msg, val.invoked);
> + err++;
> + }
> +
> + if (val.is_mptcp != 1) {
> + log_err("%s: unexpected bpf_tcp_sock.is_mptcp %d != 1",
> + msg, val.is_mptcp);
> + err++;
> + }
any reason to not use ASSERT_NEQ ?
> +
> + return err;
> +}
> +
> static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
> {
> int client_fd, prog_fd, map_fd, err;
[...]
^ permalink raw reply
* [PATCH 1/2] arm64: dts: armada-3720-turris-mox: Correct reg property for mdio devices
From: Chris Packham @ 2022-05-16 22:48 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
andrew, gregory.clement, sebastian.hesselbarth, kabel
Cc: netdev, devicetree, linux-kernel, linux-arm-kernel, Chris Packham
In-Reply-To: <20220516224801.1656752-1-chris.packham@alliedtelesis.co.nz>
MDIO devices have #address-cells = <1>, #size-cells = <0>. Now that we
have a schema enforcing this for marvell,orion-mdio we can see that the
turris-mox has a unnecessary 2nd cell for the switch nodes reg property
of it's switch devices. Remove the unnecessary 2nd cell from the
switches reg property.
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
.../boot/dts/marvell/armada-3720-turris-mox.dts | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
index 1cee26479bfe..98c9a3265446 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
@@ -303,7 +303,7 @@ phy1: ethernet-phy@1 {
/* switch nodes are enabled by U-Boot if modules are present */
switch0@10 {
compatible = "marvell,mv88e6190";
- reg = <0x10 0>;
+ reg = <0x10>;
dsa,member = <0 0>;
interrupt-parent = <&moxtet>;
interrupts = <MOXTET_IRQ_PERIDOT(0)>;
@@ -428,7 +428,7 @@ port-sfp@a {
switch0@2 {
compatible = "marvell,mv88e6085";
- reg = <0x2 0>;
+ reg = <0x2>;
dsa,member = <0 0>;
interrupt-parent = <&moxtet>;
interrupts = <MOXTET_IRQ_TOPAZ>;
@@ -495,7 +495,7 @@ port@5 {
switch1@11 {
compatible = "marvell,mv88e6190";
- reg = <0x11 0>;
+ reg = <0x11>;
dsa,member = <0 1>;
interrupt-parent = <&moxtet>;
interrupts = <MOXTET_IRQ_PERIDOT(1)>;
@@ -620,7 +620,7 @@ port-sfp@a {
switch1@2 {
compatible = "marvell,mv88e6085";
- reg = <0x2 0>;
+ reg = <0x2>;
dsa,member = <0 1>;
interrupt-parent = <&moxtet>;
interrupts = <MOXTET_IRQ_TOPAZ>;
@@ -687,7 +687,7 @@ port@5 {
switch2@12 {
compatible = "marvell,mv88e6190";
- reg = <0x12 0>;
+ reg = <0x12>;
dsa,member = <0 2>;
interrupt-parent = <&moxtet>;
interrupts = <MOXTET_IRQ_PERIDOT(2)>;
@@ -803,7 +803,7 @@ port-sfp@a {
switch2@2 {
compatible = "marvell,mv88e6085";
- reg = <0x2 0>;
+ reg = <0x2>;
dsa,member = <0 2>;
interrupt-parent = <&moxtet>;
interrupts = <MOXTET_IRQ_TOPAZ>;
--
2.36.1
^ permalink raw reply related
* [PATCH 0/2] armada-3720-turris-mox and orion-mdio
From: Chris Packham @ 2022-05-16 22:47 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
andrew, gregory.clement, sebastian.hesselbarth, kabel
Cc: netdev, devicetree, linux-kernel, linux-arm-kernel, Chris Packham
This is a follow up to the change that converted the orion-mdio dt-binding from
txt to DT schema format. At the time I thought the binding needed
'unevaluatedProperties: false' because the core mdio.yaml binding didn't handle
the DSA switches. In reality it was simply the invalid reg property causing the
downstream nodes to be unevaluated. Fixing the reg nodes means we can set
'unevaluatedProperties: true'
Marek,
I don't know if you had a change for the reg properties in flight. I didn't see
anything on lore/lkml so sorry if this crosses with something you've done.
Chris Packham (2):
arm64: dts: armada-3720-turris-mox: Correct reg property for mdio
devices
dt-bindings: net: marvell,orion-mdio: Set unevaluatedProperties to
false
.../devicetree/bindings/net/marvell,orion-mdio.yaml | 2 +-
.../boot/dts/marvell/armada-3720-turris-mox.dts | 12 ++++++------
2 files changed, 7 insertions(+), 7 deletions(-)
--
2.36.1
^ permalink raw reply
* [PATCH 2/2] dt-bindings: net: marvell,orion-mdio: Set unevaluatedProperties to false
From: Chris Packham @ 2022-05-16 22:48 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
andrew, gregory.clement, sebastian.hesselbarth, kabel
Cc: netdev, devicetree, linux-kernel, linux-arm-kernel, Chris Packham
In-Reply-To: <20220516224801.1656752-1-chris.packham@alliedtelesis.co.nz>
When the binding was converted it appeared necessary to set
'unevaluatedProperties: true' because of the switch devices on the
turris-mox board. Actually the error was because of the reg property
being incorrect causing the rest of the properties to be unevaluated.
After the reg properties are fixed for turris-mox we can set
'unevaluatedProperties: false' as is generally expected.
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml b/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
index fe3a3412f093..d2906b4a0f59 100644
--- a/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
+++ b/Documentation/devicetree/bindings/net/marvell,orion-mdio.yaml
@@ -39,7 +39,7 @@ required:
- compatible
- reg
-unevaluatedProperties: true
+unevaluatedProperties: false
examples:
- |
--
2.36.1
^ permalink raw reply related
* Re: [PATCH bpf 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
From: Eugene Syromiatnikov @ 2022-05-16 22:49 UTC (permalink / raw)
To: Jiri Olsa
Cc: Masami Hiramatsu, Steven Rostedt, Ingo Molnar, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, netdev, bpf,
linux-kernel, Shuah Khan, linux-kselftest
In-Reply-To: <YoLDdaObEQePcIN+@krava>
On Mon, May 16, 2022 at 11:34:45PM +0200, Jiri Olsa wrote:
> On Mon, May 16, 2022 at 08:27:08PM +0200, Eugene Syromiatnikov wrote:
> > + if (check_mul_overflow(cnt, sizeof(*syms), &size))
> > + return -EOVERFLOW;
>
> there was an update already:
>
> 0236fec57a15 bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link
>
> so this won't apply anymore, could you please rebase on top of the latest bpf-next/master?
The issue that this specific check has to go in 4.18, as it covers
possible out-of-bounds write, I'm not sure how to handle it, have
a branch where it is merged manually?
^ permalink raw reply
* Re: [PATCH bpf-next v2] selftests/bpf: fix building bpf selftests statically
From: patchwork-bot+netdevbpf @ 2022-05-16 22:50 UTC (permalink / raw)
To: Yosry Ahmed
Cc: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
kpsingh, haoluo, netdev, bpf, linux-kernel
In-Reply-To: <20220514002115.1376033-1-yosryahmed@google.com>
Hello:
This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:
On Sat, 14 May 2022 00:21:15 +0000 you wrote:
> bpf selftests can no longer be built with CFLAGS=-static with
> liburandom_read.so and its dependent target.
>
> Filter out -static for liburandom_read.so and its dependent target.
>
> When building statically, this leaves urandom_read relying on
> system-wide shared libraries.
>
> [...]
Here is the summary with links:
- [bpf-next,v2] selftests/bpf: fix building bpf selftests statically
https://git.kernel.org/bpf/bpf-next/c/68084a136420
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* [PATCH bpf v2 0/4] Fix 32-bit arch and compat support for the kprobe_multi attach type
From: Eugene Syromiatnikov @ 2022-05-16 23:04 UTC (permalink / raw)
To: Jiri Olsa, Masami Hiramatsu, Steven Rostedt, Ingo Molnar,
Alexei Starovoitov, Daniel Borkmann
Cc: Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, netdev, bpf, linux-kernel, Shuah Khan,
linux-kselftest
As suggested in [1], the kprobe_multi interface is to be fixed for 32-bit
architectures and compat, rather then disabled. As it turned out,
there are a couple of additional problems that are to be addressed:
- the absence of size overflow checks, leading to possible
out-of-bounds writes (addressed by the first patch);
- the assumption that long has the same size as u64, which would make
cookies arrays size calculation incorrect on 32-bit architectures
(addressed by the second patch);
- the addrs array passing API, that is incompatible with compat and has
to be changed (addressed in the fourth patch): those are kernel
addresses and not user ones (as was incorrectly stated in [2]);
this change is only semantical for 64-bit user/kernelspace,
so it shouldn't impact ABI there, at least.
[1] https://lore.kernel.org/lkml/CAADnVQ+2gwhcMht4PuDnDOFKY68Wsq8QFz4Y69NBX_TLaSexQQ@mail.gmail.com/
[2] https://lore.kernel.org/lkml/20220510184155.GA8295@asgard.redhat.com/
v2:
- Fixed the isses reported by CI
v1: https://lore.kernel.org/lkml/20220516182657.GA28596@asgard.redhat.com/
Eugene Syromiatnikov (4):
bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
bpf_trace: support 32-bit kernels in bpf_kprobe_multi_link_attach
bpf_trace: handle compat in kprobe_multi_resolve_syms
bpf_trace: pass array of u64 values in kprobe_multi.addrs
kernel/trace/bpf_trace.c | 62 ++++++++++++++++------
tools/lib/bpf/bpf.h | 2 +-
tools/lib/bpf/libbpf.c | 8 +--
tools/lib/bpf/libbpf.h | 2 +-
.../testing/selftests/bpf/prog_tests/bpf_cookie.c | 2 +-
.../selftests/bpf/prog_tests/kprobe_multi_test.c | 8 +--
6 files changed, 56 insertions(+), 28 deletions(-)
--
2.1.4
^ permalink raw reply
* [PATCH bpf v2 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
From: Eugene Syromiatnikov @ 2022-05-16 23:04 UTC (permalink / raw)
To: Jiri Olsa, Masami Hiramatsu, Steven Rostedt, Ingo Molnar,
Alexei Starovoitov, Daniel Borkmann
Cc: Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, netdev, bpf, linux-kernel, Shuah Khan,
linux-kselftest
Check that size would not overflow before calculation (and return
-EOVERFLOW if it will), to prevent potential out-of-bounds write
with the following copy_from_user. Add the same check
to kprobe_multi_resolve_syms in case it will be called from elsewhere
in the future.
Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
---
kernel/trace/bpf_trace.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index d8553f4..f1d4e68 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2352,12 +2352,15 @@ static int
kprobe_multi_resolve_syms(const void __user *usyms, u32 cnt,
unsigned long *addrs)
{
- unsigned long addr, size;
+ unsigned long addr, sym_size;
+ u32 size;
const char __user **syms;
int err = -ENOMEM;
unsigned int i;
char *func;
+ if (check_mul_overflow(cnt, (u32)sizeof(*syms), &size))
+ return -EOVERFLOW;
size = cnt * sizeof(*syms);
syms = kvzalloc(size, GFP_KERNEL);
if (!syms)
@@ -2382,9 +2385,9 @@ kprobe_multi_resolve_syms(const void __user *usyms, u32 cnt,
addr = kallsyms_lookup_name(func);
if (!addr)
goto error;
- if (!kallsyms_lookup_size_offset(addr, &size, NULL))
+ if (!kallsyms_lookup_size_offset(addr, &sym_size, NULL))
goto error;
- addr = ftrace_location_range(addr, addr + size - 1);
+ addr = ftrace_location_range(addr, addr + sym_size - 1);
if (!addr)
goto error;
addrs[i] = addr;
@@ -2429,6 +2432,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
if (!cnt)
return -EINVAL;
+ if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size))
+ return -EOVERFLOW;
size = cnt * sizeof(*addrs);
addrs = kvmalloc(size, GFP_KERNEL);
if (!addrs)
--
2.1.4
^ permalink raw reply related
* [PATCH bpf v2 2/4] bpf_trace: support 32-bit kernels in bpf_kprobe_multi_link_attach
From: Eugene Syromiatnikov @ 2022-05-16 23:05 UTC (permalink / raw)
To: Jiri Olsa, Masami Hiramatsu, Steven Rostedt, Ingo Molnar,
Alexei Starovoitov, Daniel Borkmann
Cc: Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, netdev, bpf, linux-kernel, Shuah Khan,
linux-kselftest
It seems that there is no reason not to support 32-bit architectures;
doing so requires a bit of rework with respect to cookies handling,
however, as the current code implicitly assumes
that sizeof(long) == sizeof(u64).
Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
---
kernel/trace/bpf_trace.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index f1d4e68..bf5bcfb 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2406,16 +2406,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
struct bpf_link_primer link_primer;
void __user *ucookies;
unsigned long *addrs;
- u32 flags, cnt, size;
+ u32 flags, cnt, size, cookies_size;
void __user *uaddrs;
u64 *cookies = NULL;
void __user *usyms;
int err;
- /* no support for 32bit archs yet */
- if (sizeof(u64) != sizeof(void *))
- return -EOPNOTSUPP;
-
if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI)
return -EINVAL;
@@ -2425,6 +2421,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
uaddrs = u64_to_user_ptr(attr->link_create.kprobe_multi.addrs);
usyms = u64_to_user_ptr(attr->link_create.kprobe_multi.syms);
+ ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
if (!!uaddrs == !!usyms)
return -EINVAL;
@@ -2432,8 +2429,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
if (!cnt)
return -EINVAL;
- if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size))
+ if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size) ||
+ (ucookies &&
+ check_mul_overflow(cnt, (u32)sizeof(*cookies), &cookies_size))) {
return -EOVERFLOW;
+ }
size = cnt * sizeof(*addrs);
addrs = kvmalloc(size, GFP_KERNEL);
if (!addrs)
@@ -2450,14 +2450,14 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
goto error;
}
- ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
if (ucookies) {
- cookies = kvmalloc(size, GFP_KERNEL);
+ cookies_size = cnt * sizeof(*cookies);
+ cookies = kvmalloc(cookies_size, GFP_KERNEL);
if (!cookies) {
err = -ENOMEM;
goto error;
}
- if (copy_from_user(cookies, ucookies, size)) {
+ if (copy_from_user(cookies, ucookies, cookies_size)) {
err = -EFAULT;
goto error;
}
--
2.1.4
^ permalink raw reply related
* [PATCH bpf v2 3/4] bpf_trace: handle compat in kprobe_multi_resolve_syms
From: Eugene Syromiatnikov @ 2022-05-16 23:05 UTC (permalink / raw)
To: Jiri Olsa, Masami Hiramatsu, Steven Rostedt, Ingo Molnar,
Alexei Starovoitov, Daniel Borkmann
Cc: Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, netdev, bpf, linux-kernel, Shuah Khan,
linux-kselftest
For compat processes, userspace pointer size is different. Since the
copied array is iterated anyway, the simplest fix seems to be copy the
user-supplied array as-is and the iterate as an array of native or
compat pointers, depending on the in_compat_syscall() value.
Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
---
kernel/trace/bpf_trace.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index bf5bcfb..268c92b 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2353,16 +2353,19 @@ kprobe_multi_resolve_syms(const void __user *usyms, u32 cnt,
unsigned long *addrs)
{
unsigned long addr, sym_size;
- u32 size;
+ u32 size, elem_size;
const char __user **syms;
+ compat_uptr_t __user *compat_syms;
int err = -ENOMEM;
unsigned int i;
char *func;
- if (check_mul_overflow(cnt, (u32)sizeof(*syms), &size))
+ elem_size = in_compat_syscall() ? sizeof(*compat_syms) : sizeof(*syms);
+ if (check_mul_overflow(cnt, elem_size, &size))
return -EOVERFLOW;
- size = cnt * sizeof(*syms);
+ size = cnt * elem_size;
syms = kvzalloc(size, GFP_KERNEL);
+ compat_syms = (void *)syms;
if (!syms)
return -ENOMEM;
@@ -2376,7 +2379,10 @@ kprobe_multi_resolve_syms(const void __user *usyms, u32 cnt,
}
for (i = 0; i < cnt; i++) {
- err = strncpy_from_user(func, syms[i], KSYM_NAME_LEN);
+ const char __user *ufunc = in_compat_syscall()
+ ? (char __user *)(uintptr_t)compat_syms[i]
+ : syms[i];
+ err = strncpy_from_user(func, ufunc, KSYM_NAME_LEN);
if (err == KSYM_NAME_LEN)
err = -E2BIG;
if (err < 0)
--
2.1.4
^ permalink raw reply related
* [PATCH bpf v2 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
From: Eugene Syromiatnikov @ 2022-05-16 23:05 UTC (permalink / raw)
To: Jiri Olsa, Masami Hiramatsu, Steven Rostedt, Ingo Molnar,
Alexei Starovoitov, Daniel Borkmann
Cc: Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, netdev, bpf, linux-kernel, Shuah Khan,
linux-kselftest
With the interface as defined, it is impossible to pass 64-bit kernel
addresses from a 32-bit userspace process in BPF_LINK_TYPE_KPROBE_MULTI,
which severly limits the useability of the interface, change the ABI
to accept an array of u64 values instead of (kernel? user?) longs.
Interestingly, the rest of the libbpf infrastructure uses 64-bit values
for kallsyms addresses already, so this patch also eliminates
the sym_addr cast in tools/lib/bpf/libbpf.c:resolve_kprobe_multi_cb().
Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link")
Fixes: 5117c26e877352bc ("libbpf: Add bpf_link_create support for multi kprobes")
Fixes: ddc6b04989eb0993 ("libbpf: Add bpf_program__attach_kprobe_multi_opts function")
Fixes: f7a11eeccb111854 ("selftests/bpf: Add kprobe_multi attach test")
Fixes: 9271a0c7ae7a9147 ("selftests/bpf: Add attach test for bpf_program__attach_kprobe_multi_opts")
Fixes: 2c6401c966ae1fbe ("selftests/bpf: Add kprobe_multi bpf_cookie test")
Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
---
kernel/trace/bpf_trace.c | 25 ++++++++++++++++++----
tools/lib/bpf/bpf.h | 2 +-
tools/lib/bpf/libbpf.c | 8 +++----
tools/lib/bpf/libbpf.h | 2 +-
.../testing/selftests/bpf/prog_tests/bpf_cookie.c | 2 +-
.../selftests/bpf/prog_tests/kprobe_multi_test.c | 8 +++----
6 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 268c92b..a8a639a 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2413,7 +2413,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
void __user *ucookies;
unsigned long *addrs;
u32 flags, cnt, size, cookies_size;
- void __user *uaddrs;
+ u64 __user *uaddrs;
u64 *cookies = NULL;
void __user *usyms;
int err;
@@ -2446,9 +2446,26 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
return -ENOMEM;
if (uaddrs) {
- if (copy_from_user(addrs, uaddrs, size)) {
- err = -EFAULT;
- goto error;
+ if (sizeof(*addrs) == sizeof(*uaddrs)) {
+ if (copy_from_user(addrs, uaddrs, size)) {
+ err = -EFAULT;
+ goto error;
+ }
+ } else {
+ u32 i;
+ u64 addr;
+
+ for (i = 0; i < cnt; i++) {
+ if (get_user(addr, uaddrs + i)) {
+ err = -EFAULT;
+ goto error;
+ }
+ if (addr > ULONG_MAX) {
+ err = -EINVAL;
+ goto error;
+ }
+ addrs[i] = addr;
+ }
}
} else {
err = kprobe_multi_resolve_syms(usyms, cnt, addrs);
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index f4b4afb..f677602 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -417,7 +417,7 @@ struct bpf_link_create_opts {
__u32 flags;
__u32 cnt;
const char **syms;
- const unsigned long *addrs;
+ const __u64 *addrs;
const __u64 *cookies;
} kprobe_multi;
};
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 809fe20..03a14a6 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -10279,7 +10279,7 @@ static bool glob_match(const char *str, const char *pat)
struct kprobe_multi_resolve {
const char *pattern;
- unsigned long *addrs;
+ __u64 *addrs;
size_t cap;
size_t cnt;
};
@@ -10294,12 +10294,12 @@ resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type,
if (!glob_match(sym_name, res->pattern))
return 0;
- err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(unsigned long),
+ err = libbpf_ensure_mem((void **) &res->addrs, &res->cap, sizeof(__u64),
res->cnt + 1);
if (err)
return err;
- res->addrs[res->cnt++] = (unsigned long) sym_addr;
+ res->addrs[res->cnt++] = sym_addr;
return 0;
}
@@ -10314,7 +10314,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
};
struct bpf_link *link = NULL;
char errmsg[STRERR_BUFSIZE];
- const unsigned long *addrs;
+ const __u64 *addrs;
int err, link_fd, prog_fd;
const __u64 *cookies;
const char **syms;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 05dde85..ec1cb61 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -431,7 +431,7 @@ struct bpf_kprobe_multi_opts {
/* array of function symbols to attach */
const char **syms;
/* array of function addresses to attach */
- const unsigned long *addrs;
+ const __u64 *addrs;
/* array of user-provided values fetchable through bpf_get_attach_cookie */
const __u64 *cookies;
/* number of elements in syms/addrs/cookies arrays */
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
index 923a613..5aa482a 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
@@ -137,7 +137,7 @@ static void kprobe_multi_link_api_subtest(void)
cookies[6] = 7;
cookies[7] = 8;
- opts.kprobe_multi.addrs = (const unsigned long *) &addrs;
+ opts.kprobe_multi.addrs = (const __u64 *) &addrs;
opts.kprobe_multi.cnt = ARRAY_SIZE(addrs);
opts.kprobe_multi.cookies = (const __u64 *) &cookies;
prog_fd = bpf_program__fd(skel->progs.test_kprobe);
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
index b9876b5..fbf4cf2 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -105,7 +105,7 @@ static void test_link_api_addrs(void)
GET_ADDR("bpf_fentry_test7", addrs[6]);
GET_ADDR("bpf_fentry_test8", addrs[7]);
- opts.kprobe_multi.addrs = (const unsigned long*) addrs;
+ opts.kprobe_multi.addrs = (const __u64 *) addrs;
opts.kprobe_multi.cnt = ARRAY_SIZE(addrs);
test_link_api(&opts);
}
@@ -183,7 +183,7 @@ static void test_attach_api_addrs(void)
GET_ADDR("bpf_fentry_test7", addrs[6]);
GET_ADDR("bpf_fentry_test8", addrs[7]);
- opts.addrs = (const unsigned long *) addrs;
+ opts.addrs = (const __u64 *) addrs;
opts.cnt = ARRAY_SIZE(addrs);
test_attach_api(NULL, &opts);
}
@@ -241,7 +241,7 @@ static void test_attach_api_fails(void)
goto cleanup;
/* fail_2 - both addrs and syms set */
- opts.addrs = (const unsigned long *) addrs;
+ opts.addrs = (const __u64 *) addrs;
opts.syms = syms;
opts.cnt = ARRAY_SIZE(syms);
opts.cookies = NULL;
@@ -255,7 +255,7 @@ static void test_attach_api_fails(void)
goto cleanup;
/* fail_3 - pattern and addrs set */
- opts.addrs = (const unsigned long *) addrs;
+ opts.addrs = (const __u64 *) addrs;
opts.syms = NULL;
opts.cnt = ARRAY_SIZE(syms);
opts.cookies = NULL;
--
2.1.4
^ permalink raw reply related
* Re: [PATCH net] amt: fix gateway mode stuck
From: Jakub Kicinski @ 2022-05-16 23:10 UTC (permalink / raw)
To: Taehee Yoo; +Cc: davem, pabeni, edumazet, netdev
In-Reply-To: <20220514131346.17045-1-ap420073@gmail.com>
On Sat, 14 May 2022 13:13:46 +0000 Taehee Yoo wrote:
> - if (amt_advertisement_handler(amt, skb))
> + err = amt_advertisement_handler(amt, skb);
> + if (err)
> amt->dev->stats.rx_dropped++;
> - goto out;
> + break;
There's another amt->dev->stats.rx_dropped++; before the end of this
function which now won't be skipped, I think you're counting twice.
^ permalink raw reply
* Re: [PATCH v3] bpf: Fix KASAN use-after-free Read in compute_effective_progs
From: Andrii Nakryiko @ 2022-05-16 23:16 UTC (permalink / raw)
To: Tadeusz Struk
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Networking, bpf, linux- stable, open list,
syzbot+f264bffdfbd5614f3bb2
In-Reply-To: <20220513190821.431762-1-tadeusz.struk@linaro.org>
On Fri, May 13, 2022 at 12:08 PM Tadeusz Struk <tadeusz.struk@linaro.org> wrote:
>
> Syzbot found a Use After Free bug in compute_effective_progs().
> The reproducer creates a number of BPF links, and causes a fault
> injected alloc to fail, while calling bpf_link_detach on them.
> Link detach triggers the link to be freed by bpf_link_free(),
> which calls __cgroup_bpf_detach() and update_effective_progs().
> If the memory allocation in this function fails, the function restores
> the pointer to the bpf_cgroup_link on the cgroup list, but the memory
> gets freed just after it returns. After this, every subsequent call to
> update_effective_progs() causes this already deallocated pointer to be
> dereferenced in prog_list_length(), and triggers KASAN UAF error.
>
> To fix this issue don't preserve the pointer to the prog or link in the
> list, but remove it and replace it with a dummy prog without shrinking
> the table. The subsequent call to __cgroup_bpf_detach() or
> __cgroup_bpf_detach() will correct it.
>
> Cc: "Alexei Starovoitov" <ast@kernel.org>
> Cc: "Daniel Borkmann" <daniel@iogearbox.net>
> Cc: "Andrii Nakryiko" <andrii@kernel.org>
> Cc: "Martin KaFai Lau" <kafai@fb.com>
> Cc: "Song Liu" <songliubraving@fb.com>
> Cc: "Yonghong Song" <yhs@fb.com>
> Cc: "John Fastabend" <john.fastabend@gmail.com>
> Cc: "KP Singh" <kpsingh@kernel.org>
> Cc: <netdev@vger.kernel.org>
> Cc: <bpf@vger.kernel.org>
> Cc: <stable@vger.kernel.org>
> Cc: <linux-kernel@vger.kernel.org>
>
> Link: https://syzkaller.appspot.com/bug?id=8ebf179a95c2a2670f7cf1ba62429ec044369db4
> Fixes: af6eea57437a ("bpf: Implement bpf_link-based cgroup BPF program attachment")
> Reported-by: <syzbot+f264bffdfbd5614f3bb2@syzkaller.appspotmail.com>
> Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org>
> ---
> v2: Add a fall back path that removes a prog from the effective progs
> table in case detach fails to allocate memory in compute_effective_progs().
>
> v3: Implement the fallback in a separate function purge_effective_progs
> ---
> kernel/bpf/cgroup.c | 64 +++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 56 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 128028efda64..9d3af4d6c055 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -681,6 +681,57 @@ static struct bpf_prog_list *find_detach_entry(struct list_head *progs,
> return ERR_PTR(-ENOENT);
> }
>
> +/**
> + * purge_effective_progs() - After compute_effective_progs fails to alloc new
> + * cgrp->bpf.inactive table we can recover by
> + * recomputing the array in place.
> + *
> + * @cgrp: The cgroup which descendants to traverse
> + * @link: A link to detach
> + * @atype: Type of detach operation
> + */
> +static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
> + enum cgroup_bpf_attach_type atype)
> +{
> + struct cgroup_subsys_state *css;
> + struct bpf_prog_array_item *item;
> + struct bpf_prog *tmp;
> + struct bpf_prog_array *array;
> + int index = 0, index_purge = -1;
> +
> + if (!prog)
> + return;
> +
> + /* recompute effective prog array in place */
> + css_for_each_descendant_pre(css, &cgrp->self) {
> + struct cgroup *desc = container_of(css, struct cgroup, self);
> +
> + array = desc->bpf.effective[atype];
../kernel/bpf/cgroup.c:748:23: warning: incorrect type in assignment
(different address spaces)
../kernel/bpf/cgroup.c:748:23: expected struct bpf_prog_array *array
../kernel/bpf/cgroup.c:748:23: got struct bpf_prog_array [noderef] __rcu *
you need rcu_dereference here? but also see suggestions below to avoid
iterating effective directly (it's ambiguous to search by prog only)
> + item = &array->items[0];
> +
> + /* Find the index of the prog to purge */
> + while ((tmp = READ_ONCE(item->prog))) {
> + if (tmp == prog) {
I think comparing just prog isn't always correct, as the same program
can be in effective array multiple times if attached through bpf_link.
Looking at replace_effective_prog() I think we can do a very similar
(and tested) approach:
1. restore original pl state in __cgroup_bpf_detach (so we can find it
by comparing pl->prog == prog && pl->link == link)
2. use replace_effective_prog's approach to find position of pl in
effective array (using this nested for loop over cgroup parents and
list_for_each_entry inside)
3. then instead of replacing one prog with another do
bpf_prog_array_delete_safe_at ?
I'd feel more comfortable using the same tested overall approach of
replace_effective_prog.
> + index_purge = index;
> + break;
> + }
> + item++;
> + index++;
> + }
> +
> + /* Check if we found what's needed for removing the prog */
> + if (index_purge == -1 || index_purge == index - 1)
> + continue;
the search shouldn't fail, should it?
> +
> + /* Remove the program from the array */
> + WARN_ONCE(bpf_prog_array_delete_safe_at(array, index_purge),
> + "Failed to purge a prog from array at index %d", index_purge);
> +
> + index = 0;
> + index_purge = -1;
> + }
> +}
> +
> /**
> * __cgroup_bpf_detach() - Detach the program or link from a cgroup, and
> * propagate the change to descendants
> @@ -723,8 +774,11 @@ static int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
> pl->link = NULL;
>
> err = update_effective_progs(cgrp, atype);
> - if (err)
> - goto cleanup;
> + if (err) {
> + struct bpf_prog *prog_purge = prog ? prog : link->link.prog;
> +
so here we shouldn't forget link, instead pass both link and prog (one
of them will have to be NULL) into purge_effective_progs
> + purge_effective_progs(cgrp, prog_purge, atype);
> + }
>
> /* now can actually delete it from this cgroup list */
> list_del(&pl->node);
> @@ -736,12 +790,6 @@ static int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
> bpf_prog_put(old_prog);
> static_branch_dec(&cgroup_bpf_enabled_key[atype]);
> return 0;
> -
> -cleanup:
> - /* restore back prog or link */
> - pl->prog = old_prog;
> - pl->link = link;
> - return err;
> }
>
> static int cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
> --
> 2.36.1
>
^ permalink raw reply
* Re: [PATCH v3] bpf: Fix KASAN use-after-free Read in compute_effective_progs
From: Tadeusz Struk @ 2022-05-16 23:35 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Networking, bpf, linux- stable, open list,
syzbot+f264bffdfbd5614f3bb2
In-Reply-To: <CAEf4BzY-p13huoqo6N7LJRVVj8rcjPeP3Cp=KDX4N2x9BkC9Zw@mail.gmail.com>
On 5/16/22 16:16, Andrii Nakryiko wrote:
> On Fri, May 13, 2022 at 12:08 PM Tadeusz Struk <tadeusz.struk@linaro.org> wrote:
>> kernel/bpf/cgroup.c | 64 +++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 56 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index 128028efda64..9d3af4d6c055 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -681,6 +681,57 @@ static struct bpf_prog_list *find_detach_entry(struct list_head *progs,
>> return ERR_PTR(-ENOENT);
>> }
>>
>> +/**
>> + * purge_effective_progs() - After compute_effective_progs fails to alloc new
>> + * cgrp->bpf.inactive table we can recover by
>> + * recomputing the array in place.
>> + *
>> + * @cgrp: The cgroup which descendants to traverse
>> + * @link: A link to detach
>> + * @atype: Type of detach operation
>> + */
>> +static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
>> + enum cgroup_bpf_attach_type atype)
>> +{
>> + struct cgroup_subsys_state *css;
>> + struct bpf_prog_array_item *item;
>> + struct bpf_prog *tmp;
>> + struct bpf_prog_array *array;
>> + int index = 0, index_purge = -1;
>> +
>> + if (!prog)
>> + return;
>> +
>> + /* recompute effective prog array in place */
>> + css_for_each_descendant_pre(css, &cgrp->self) {
>> + struct cgroup *desc = container_of(css, struct cgroup, self);
>> +
>> + array = desc->bpf.effective[atype];
>
> ../kernel/bpf/cgroup.c:748:23: warning: incorrect type in assignment
> (different address spaces)
> ../kernel/bpf/cgroup.c:748:23: expected struct bpf_prog_array *array
> ../kernel/bpf/cgroup.c:748:23: got struct bpf_prog_array [noderef] __rcu *
>
>
> you need rcu_dereference here? but also see suggestions below to avoid
> iterating effective directly (it's ambiguous to search by prog only)
I didn't check it with sparse so I didn't see this warning.
Will fix in the next version.
>
>> + item = &array->items[0];
>> +
>> + /* Find the index of the prog to purge */
>> + while ((tmp = READ_ONCE(item->prog))) {
>> + if (tmp == prog) {
>
> I think comparing just prog isn't always correct, as the same program
> can be in effective array multiple times if attached through bpf_link.
>
> Looking at replace_effective_prog() I think we can do a very similar
> (and tested) approach:
>
> 1. restore original pl state in __cgroup_bpf_detach (so we can find it
> by comparing pl->prog == prog && pl->link == link)
> 2. use replace_effective_prog's approach to find position of pl in
> effective array (using this nested for loop over cgroup parents and
> list_for_each_entry inside)
> 3. then instead of replacing one prog with another do
> bpf_prog_array_delete_safe_at ?
>
> I'd feel more comfortable using the same tested overall approach of
> replace_effective_prog.
Ok, I can try that.
>
>> + index_purge = index;
>> + break;
>> + }
>> + item++;
>> + index++;
>> + }
>> +
>> + /* Check if we found what's needed for removing the prog */
>> + if (index_purge == -1 || index_purge == index - 1)
>> + continue;
>
> the search shouldn't fail, should it?
I wasn't if the prog will be present in all parents so I decided to add this
check to make sure it is found.
>
>> +
>> + /* Remove the program from the array */
>> + WARN_ONCE(bpf_prog_array_delete_safe_at(array, index_purge),
>> + "Failed to purge a prog from array at index %d", index_purge);
>> +
>> + index = 0;
>> + index_purge = -1;
>> + }
>> +}
>> +
>> /**
>> * __cgroup_bpf_detach() - Detach the program or link from a cgroup, and
>> * propagate the change to descendants
>> @@ -723,8 +774,11 @@ static int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
>> pl->link = NULL;
>>
>> err = update_effective_progs(cgrp, atype);
>> - if (err)
>> - goto cleanup;
>> + if (err) {
>> + struct bpf_prog *prog_purge = prog ? prog : link->link.prog;
>> +
>
> so here we shouldn't forget link, instead pass both link and prog (one
> of them will have to be NULL) into purge_effective_progs
ok, I will pass in both.
--
Thanks,
Tadeusz
^ permalink raw reply
* Re: [PATCH v2 4/4] powerpc/52xx: Convert to use fwnode API
From: Michael Ellerman @ 2022-05-16 23:38 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Wolfram Sang, Marc Kleine-Budde, Damien Le Moal, Mark Brown,
chris.packham, Sergey Shtylyov, David S. Miller, Jakub Kicinski,
Greg Kroah-Hartman, Jiri Slaby, linuxppc-dev, linux-kernel,
linux-ide, linux-i2c, linux-can, netdev, linux-spi, linux-serial,
Benjamin Herrenschmidt, Paul Mackerras, Anatolij Gustschin,
Wolfgang Grandegger, Eric Dumazet, Paolo Abeni, Pantelis Antoniou
In-Reply-To: <YoJbaTNJFV2A1Etw@smile.fi.intel.com>
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> On Mon, May 16, 2022 at 05:05:12PM +0300, Andy Shevchenko wrote:
>> On Mon, May 16, 2022 at 11:48:05PM +1000, Michael Ellerman wrote:
>> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > > We may convert the GPT driver to use fwnode API for the sake
>> > > of consistency of the used APIs inside the driver.
>> >
>> > I'm not sure about this one.
>> >
>> > It's more consistent to use fwnode in this driver, but it's very
>> > inconsistent with the rest of the powerpc code. We have basically no
>> > uses of the fwnode APIs at the moment.
>>
>> Fair point!
>>
>> > It seems like a pretty straight-forward conversion, but there could
>> > easily be a bug in there, I don't have any way to test it. Do you?
>>
>> Nope, only compile testing. The important part of this series is to
>> clean up of_node from GPIO library, so since here it's a user of
>> it I want to do that. This patch is just ad-hoc conversion that I
>> noticed is possible. But there is no any requirement to do so.
>>
>> Lemme drop this from v3.
>
> I just realize that there is no point to send a v3. You can just apply
> first 3 patches. Or is your comment against entire series?
No, my comment is just about this patch.
I don't mind converting to new APIs when it's blocking some other
cleanup. But given the age of this code I think it's probably better to
just leave the rest of it as-is, unless someone volunteers to test it.
So yeah I'll just take patches 1-3 of this v2 series, no need to resend.
cheers
^ permalink raw reply
* Re: [PATCH v3] bpf: Fix KASAN use-after-free Read in compute_effective_progs
From: Andrii Nakryiko @ 2022-05-16 23:45 UTC (permalink / raw)
To: Tadeusz Struk
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Networking, bpf, linux- stable, open list,
syzbot+f264bffdfbd5614f3bb2
In-Reply-To: <2fcdbecf-5352-ea81-ee42-ee10fbe2f72e@linaro.org>
On Mon, May 16, 2022 at 4:35 PM Tadeusz Struk <tadeusz.struk@linaro.org> wrote:
>
> On 5/16/22 16:16, Andrii Nakryiko wrote:
> > On Fri, May 13, 2022 at 12:08 PM Tadeusz Struk <tadeusz.struk@linaro.org> wrote:
> >> kernel/bpf/cgroup.c | 64 +++++++++++++++++++++++++++++++++++++++------
> >> 1 file changed, 56 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> >> index 128028efda64..9d3af4d6c055 100644
> >> --- a/kernel/bpf/cgroup.c
> >> +++ b/kernel/bpf/cgroup.c
> >> @@ -681,6 +681,57 @@ static struct bpf_prog_list *find_detach_entry(struct list_head *progs,
> >> return ERR_PTR(-ENOENT);
> >> }
> >>
> >> +/**
> >> + * purge_effective_progs() - After compute_effective_progs fails to alloc new
> >> + * cgrp->bpf.inactive table we can recover by
> >> + * recomputing the array in place.
> >> + *
> >> + * @cgrp: The cgroup which descendants to traverse
> >> + * @link: A link to detach
> >> + * @atype: Type of detach operation
> >> + */
> >> +static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
> >> + enum cgroup_bpf_attach_type atype)
> >> +{
> >> + struct cgroup_subsys_state *css;
> >> + struct bpf_prog_array_item *item;
> >> + struct bpf_prog *tmp;
> >> + struct bpf_prog_array *array;
> >> + int index = 0, index_purge = -1;
> >> +
> >> + if (!prog)
> >> + return;
> >> +
> >> + /* recompute effective prog array in place */
> >> + css_for_each_descendant_pre(css, &cgrp->self) {
> >> + struct cgroup *desc = container_of(css, struct cgroup, self);
> >> +
> >> + array = desc->bpf.effective[atype];
> >
> > ../kernel/bpf/cgroup.c:748:23: warning: incorrect type in assignment
> > (different address spaces)
> > ../kernel/bpf/cgroup.c:748:23: expected struct bpf_prog_array *array
> > ../kernel/bpf/cgroup.c:748:23: got struct bpf_prog_array [noderef] __rcu *
> >
> >
> > you need rcu_dereference here? but also see suggestions below to avoid
> > iterating effective directly (it's ambiguous to search by prog only)
>
> I didn't check it with sparse so I didn't see this warning.
> Will fix in the next version.
>
> >
> >> + item = &array->items[0];
> >> +
> >> + /* Find the index of the prog to purge */
> >> + while ((tmp = READ_ONCE(item->prog))) {
> >> + if (tmp == prog) {
> >
> > I think comparing just prog isn't always correct, as the same program
> > can be in effective array multiple times if attached through bpf_link.
> >
> > Looking at replace_effective_prog() I think we can do a very similar
> > (and tested) approach:
> >
> > 1. restore original pl state in __cgroup_bpf_detach (so we can find it
> > by comparing pl->prog == prog && pl->link == link)
> > 2. use replace_effective_prog's approach to find position of pl in
> > effective array (using this nested for loop over cgroup parents and
> > list_for_each_entry inside)
> > 3. then instead of replacing one prog with another do
> > bpf_prog_array_delete_safe_at ?
> >
> > I'd feel more comfortable using the same tested overall approach of
> > replace_effective_prog.
>
> Ok, I can try that.
>
> >
> >> + index_purge = index;
> >> + break;
> >> + }
> >> + item++;
> >> + index++;
> >> + }
> >> +
> >> + /* Check if we found what's needed for removing the prog */
> >> + if (index_purge == -1 || index_purge == index - 1)
> >> + continue;
> >
> > the search shouldn't fail, should it?
>
> I wasn't if the prog will be present in all parents so I decided to add this
> check to make sure it is found.
Looking at replace_effective_prog (it's been a while since I touched
this code) it has to be present, otherwise it's a bug
>
> >
> >> +
> >> + /* Remove the program from the array */
> >> + WARN_ONCE(bpf_prog_array_delete_safe_at(array, index_purge),
> >> + "Failed to purge a prog from array at index %d", index_purge);
> >> +
> >> + index = 0;
> >> + index_purge = -1;
> >> + }
> >> +}
> >> +
> >> /**
> >> * __cgroup_bpf_detach() - Detach the program or link from a cgroup, and
> >> * propagate the change to descendants
> >> @@ -723,8 +774,11 @@ static int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
> >> pl->link = NULL;
> >>
> >> err = update_effective_progs(cgrp, atype);
> >> - if (err)
> >> - goto cleanup;
> >> + if (err) {
> >> + struct bpf_prog *prog_purge = prog ? prog : link->link.prog;
> >> +
> >
> > so here we shouldn't forget link, instead pass both link and prog (one
> > of them will have to be NULL) into purge_effective_progs
>
> ok, I will pass in both.
>
> --
> Thanks,
> Tadeusz
^ permalink raw reply
* Re: [PATCH net-next 1/9] can: raw: raw_sendmsg(): remove not needed setting of skb->sk
From: patchwork-bot+netdevbpf @ 2022-05-17 0:10 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: netdev, davem, kuba, linux-can, kernel, socketcan
In-Reply-To: <20220516202625.1129281-2-mkl@pengutronix.de>
Hello:
This series was applied to netdev/net-next.git (master)
by Marc Kleine-Budde <mkl@pengutronix.de>:
On Mon, 16 May 2022 22:26:17 +0200 you wrote:
> The skb in raw_sendmsg() is allocated with sock_alloc_send_skb(),
> which subsequently calls sock_alloc_send_pskb() -> skb_set_owner_w(),
> which assigns "skb->sk = sk".
>
> This patch removes the not needed setting of skb->sk.
>
> Link: https://lore.kernel.org/all/20220502091946.1916211-2-mkl@pengutronix.de
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>
> [...]
Here is the summary with links:
- [net-next,1/9] can: raw: raw_sendmsg(): remove not needed setting of skb->sk
https://git.kernel.org/netdev/net-next/c/2af84932b3a1
- [net-next,2/9] can: raw: add support for SO_TXTIME/SCM_TXTIME
https://git.kernel.org/netdev/net-next/c/51a0d5e51178
- [net-next,3/9] can: isotp: add support for transmission without flow control
https://git.kernel.org/netdev/net-next/c/9f39d36530e5
- [net-next,4/9] can: isotp: isotp_bind(): return -EINVAL on incorrect CAN ID formatting
https://git.kernel.org/netdev/net-next/c/2aa39889c463
- [net-next,5/9] can: ctucanfd: Let users select instead of depend on CAN_CTUCANFD
https://git.kernel.org/netdev/net-next/c/94737ef56b61
- [net-next,6/9] can: slcan: slc_xmit(): use can_dropped_invalid_skb() instead of manual check
https://git.kernel.org/netdev/net-next/c/30abc9291329
- [net-next,7/9] dt-bindings: can: renesas,rcar-canfd: Make interrupt-names required
https://git.kernel.org/netdev/net-next/c/48b171dbf7b6
- [net-next,8/9] dt-bindings: can: ctucanfd: include common CAN controller bindings
https://git.kernel.org/netdev/net-next/c/14e1e9338c08
- [net-next,9/9] docs: ctucanfd: Use 'kernel-figure' directive instead of 'figure'
https://git.kernel.org/netdev/net-next/c/ba3e2eaef1ae
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH net] amt: fix gateway mode stuck
From: Taehee Yoo @ 2022-05-17 0:18 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, pabeni, edumazet, netdev
In-Reply-To: <20220516161001.78b3b49b@kernel.org>
On 5/17/22 08:10, Jakub Kicinski wrote:
Hi Jakub,
Thanks a lot for your review!
> On Sat, 14 May 2022 13:13:46 +0000 Taehee Yoo wrote:
>> - if (amt_advertisement_handler(amt, skb))
>> + err = amt_advertisement_handler(amt, skb);
>> + if (err)
>> amt->dev->stats.rx_dropped++;
>> - goto out;
>> + break;
>
> There's another amt->dev->stats.rx_dropped++; before the end of this
> function which now won't be skipped, I think you're counting twice.
This is intended.
It skips a remaining handling of advertisement message.
So, I think a memory leak would occur at this point, so I added.
Thanks!
Taehee Yoo
^ permalink raw reply
* Re: [PATCH net-next v3 02/10] ptp: ocp: add Celestica timecard PCI ids
From: Jakub Kicinski @ 2022-05-17 0:43 UTC (permalink / raw)
To: Jonathan Lemon
Cc: netdev, richardcochran, davem, pabeni, edumazet, kernel-team
In-Reply-To: <20220513225924.1655-3-jonathan.lemon@gmail.com>
On Fri, 13 May 2022 15:59:16 -0700 Jonathan Lemon wrote:
> +#ifndef PCI_VENDOR_ID_CELESTICA
> +#define PCI_VENDOR_ID_CELESTICA 0x18d4
> +#endif
> +
> +#ifndef PCI_DEVICE_ID_CELESTICA_TIMECARD
> +#define PCI_DEVICE_ID_CELESTICA_TIMECARD 0x1008
> +#endif
The ifdefs are unnecessary, these kind of constructs are often used out
of tree when one does not control the headers, but not sure what purpose
they'd serve upstream?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox