* [PATCH net-next 8/9] dt-bindings: can: ctucanfd: include common CAN controller bindings
From: Marc Kleine-Budde @ 2022-05-16 20:26 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Marc Kleine-Budde, Ondrej Ille,
Pavel Pisa, Rob Herring
In-Reply-To: <20220516202625.1129281-1-mkl@pengutronix.de>
Since commit
| 1f9234401ce0 ("dt-bindings: can: add can-controller.yaml")
there is a common CAN controller binding. Add this to the ctucanfd
binding.
Cc: Ondrej Ille <ondrej.ille@gmail.com>
Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml b/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml
index fb34d971dcb3..4635cb96fc64 100644
--- a/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml
+++ b/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml
@@ -25,6 +25,9 @@ maintainers:
- Ondrej Ille <ondrej.ille@gmail.com>
- Martin Jerabek <martin.jerabek01@gmail.com>
+allOf:
+ - $ref: can-controller.yaml#
+
properties:
compatible:
oneOf:
--
2.35.1
^ permalink raw reply related
* [PATCH net-next 7/9] dt-bindings: can: renesas,rcar-canfd: Make interrupt-names required
From: Marc Kleine-Budde @ 2022-05-16 20:26 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, linux-can, kernel, Geert Uytterhoeven,
Marc Kleine-Budde
In-Reply-To: <20220516202625.1129281-1-mkl@pengutronix.de>
From: Geert Uytterhoeven <geert+renesas@glider.be>
The Renesas R-Car CAN FD Controller always uses two or more interrupts.
Make the interrupt-names properties a required property, to make it
easier to identify the individual interrupts.
Update the example accordingly.
Link: https://lore.kernel.org/all/a68e65955e0df4db60233d468f348203c2e7b940.1651512451.git.geert+renesas@glider.be
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
.../devicetree/bindings/net/can/renesas,rcar-canfd.yaml | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
index 9fc137fafed9..6f71fc96bc4e 100644
--- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
+++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
@@ -88,6 +88,7 @@ required:
- compatible
- reg
- interrupts
+ - interrupt-names
- clocks
- clock-names
- power-domains
@@ -136,7 +137,6 @@ then:
- const: rstc_n
required:
- - interrupt-names
- reset-names
else:
properties:
@@ -167,6 +167,7 @@ examples:
reg = <0xe66c0000 0x8000>;
interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "ch_int", "g_int";
clocks = <&cpg CPG_MOD 914>,
<&cpg CPG_CORE R8A7795_CLK_CANFD>,
<&can_clk>;
--
2.35.1
^ permalink raw reply related
* Re: [PATCH net-next 0/3] mptcp: Updates for net-next
From: patchwork-bot+netdevbpf @ 2022-05-16 20:20 UTC (permalink / raw)
To: Mat Martineau
Cc: netdev, davem, kuba, pabeni, edumazet, matthieu.baerts, mptcp
In-Reply-To: <20220514002115.725976-1-mathew.j.martineau@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 17:21:12 -0700 you wrote:
> Three independent fixes/features from the MPTCP tree:
>
> Patch 1 is a selftest workaround for older iproute2 packages.
>
> Patch 2 removes superfluous locks that were added with recent MP_FAIL
> patches.
>
> [...]
Here is the summary with links:
- [net-next,1/3] selftests: mptcp: fix a mp_fail test warning
https://git.kernel.org/netdev/net-next/c/c43ce39870b3
- [net-next,2/3] Revert "mptcp: add data lock for sk timers"
https://git.kernel.org/netdev/net-next/c/0ea5374255a9
- [net-next,3/3] mptcp: sockopt: add TCP_DEFER_ACCEPT support
https://git.kernel.org/netdev/net-next/c/ea1e301d04b7
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 v2] ptp: ocp: have adjtime handle negative delta_ns correctly
From: patchwork-bot+netdevbpf @ 2022-05-16 20:20 UTC (permalink / raw)
To: Jonathan Lemon
Cc: netdev, vfedorenko, richardcochran, davem, kuba, pabeni, edumazet,
kernel-team
In-Reply-To: <20220513225231.1412-1-jonathan.lemon@gmail.com>
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Fri, 13 May 2022 15:52:31 -0700 you wrote:
> delta_ns is a s64, but it was being passed ptp_ocp_adjtime_coarse
> as an u64. Also, it turns out that timespec64_add_ns() only handles
> positive values, so perform the math with set_normalized_timespec().
>
> Fixes: 90f8f4c0e3ce ("ptp: ocp: Add ptp_ocp_adjtime_coarse for large adjustments")
> Suggested-by: Vadim Fedorenko <vfedorenko@novek.ru>
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>
> [...]
Here is the summary with links:
- [net,v2] ptp: ocp: have adjtime handle negative delta_ns correctly
https://git.kernel.org/netdev/net/c/da2172a9bfec
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-next v3] net: dsa: realtek: rtl8366rb: Serialize indirect PHY register access
From: patchwork-bot+netdevbpf @ 2022-05-16 20:20 UTC (permalink / raw)
To: Linus Walleij
Cc: andrew, vivien.didelot, f.fainelli, olteanv, davem, kuba, netdev,
alsi, lkp
In-Reply-To: <20220513213618.2742895-1-linus.walleij@linaro.org>
Hello:
This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Fri, 13 May 2022 23:36:18 +0200 you wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>
> Lock the regmap during the whole PHY register access routines in
> rtl8366rb.
>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> Reported-by: kernel test robot <lkp@intel.com>
> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> [...]
Here is the summary with links:
- [net-next,v3] net: dsa: realtek: rtl8366rb: Serialize indirect PHY register access
https://git.kernel.org/netdev/net-next/c/f008f8d0305c
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-next 0/2] net: skb: Remove skb_data_area_size()
From: Jakub Kicinski @ 2022-05-16 20:16 UTC (permalink / raw)
To: Ricardo Martinez
Cc: netdev, linux-wireless, 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>
On Fri, 13 May 2022 10:33:58 -0700 Ricardo Martinez 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/
Thanks for following up!
^ permalink raw reply
* Re: [PATCH net-next 1/3] selftests: mptcp: fix a mp_fail test warning
From: Jakub Kicinski @ 2022-05-16 20:13 UTC (permalink / raw)
To: Mat Martineau
Cc: netdev, Geliang Tang, davem, pabeni, edumazet, matthieu.baerts,
mptcp
In-Reply-To: <20220514002115.725976-2-mathew.j.martineau@linux.intel.com>
On Fri, 13 May 2022 17:21:13 -0700 Mat Martineau wrote:
> tc -n $ns2 -j -s action show action pedit index 100 | \
> + grep "packets" | \
> sed 's/.*"packets":\([0-9]\+\),.*/\1/'
sed can do the grepping for you:
sed -n 's/.*"packets":\([0-9]\+\),.*/\1/p'
But really grepping JSON output seems weird. Why not use jq?
^ permalink raw reply
* Re: [PATCH net-next v3 03/10] udp/ipv6: prioritise the ip6 path over ip4 checks
From: Pavel Begunkov @ 2022-05-16 20:10 UTC (permalink / raw)
To: Paolo Abeni, netdev, David S . Miller, Jakub Kicinski
Cc: David Ahern, Eric Dumazet, linux-kernel
In-Reply-To: <f0fb2ffbde15b2939ed76545b549bdcd33b92ae8.camel@redhat.com>
On 5/16/22 14:14, Paolo Abeni wrote:
> On Fri, 2022-05-13 at 16:26 +0100, Pavel Begunkov wrote:
>> For AF_INET6 sockets we care the most about ipv6 but not ip4 mappings as
>> it's requires some extra hops anyway. Take AF_INET6 case from the address
>> parsing switch and add an explicit path for it. It removes some extra
>> ifs from the path and removes the switch overhead.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>> net/ipv6/udp.c | 37 +++++++++++++++++--------------------
>> 1 file changed, 17 insertions(+), 20 deletions(-)
>>
>> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
>> index 85bff1252f5c..e0b1bea998ce 100644
>> --- a/net/ipv6/udp.c
>> +++ b/net/ipv6/udp.c
>> @@ -1360,30 +1360,27 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>>
>> /* destination address check */
>> if (sin6) {
>> - if (addr_len < offsetof(struct sockaddr, sa_data))
>> - return -EINVAL;
>> + if (addr_len < SIN6_LEN_RFC2133 || sin6->sin6_family != AF_INET6) {
>> + if (addr_len < offsetof(struct sockaddr, sa_data))
>> + return -EINVAL;
>
> I think you can't access 'sin6->sin6_family' before validating the
> socket address len, that is before doing:
Paolo, thanks for reviewing it!
sin6_family is protected by
if (addr_len < SIN6_LEN_RFC2133 ...)
on the previous line. I can add a BUILD_BUG_ON() if that
would be more reassuring.
>
> if (addr_len < offsetof(struct sockaddr, sa_data))
--
Pavel Begunkov
^ permalink raw reply
* Re: [PATCH net-next v3 02/10] udp/ipv6: move pending section of udpv6_sendmsg
From: Pavel Begunkov @ 2022-05-16 20:09 UTC (permalink / raw)
To: Paolo Abeni, netdev, David S . Miller, Jakub Kicinski
Cc: David Ahern, Eric Dumazet, linux-kernel
In-Reply-To: <b9844f3ce486c5aff8547e79abf4344488db6568.camel@redhat.com>
On 5/16/22 14:11, Paolo Abeni wrote:
> On Fri, 2022-05-13 at 16:26 +0100, Pavel Begunkov wrote:
>> Move up->pending section of udpv6_sendmsg() to the beginning of the
>> function. Even though it require some code duplication for sin6 parsing,
>> it clearly localises the pending handling in one place, removes an extra
>> if and more importantly will prepare the code for further patches.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>> net/ipv6/udp.c | 70 ++++++++++++++++++++++++++++++--------------------
>> 1 file changed, 42 insertions(+), 28 deletions(-)
>>
>> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
>> index 11d44ed46953..85bff1252f5c 100644
>> --- a/net/ipv6/udp.c
>> +++ b/net/ipv6/udp.c
>> @@ -1318,6 +1318,46 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>> ipc6.sockc.tsflags = sk->sk_tsflags;
>> ipc6.sockc.mark = sk->sk_mark;
>>
>> + /* Rough check on arithmetic overflow,
>> + better check is made in ip6_append_data().
>> + */
>> + if (unlikely(len > INT_MAX - sizeof(struct udphdr)))
>> + return -EMSGSIZE;
>> +
>> + getfrag = is_udplite ? udplite_getfrag : ip_generic_getfrag;
>> +
>> + /* There are pending frames. */
>> + if (up->pending) {
>> + if (up->pending == AF_INET)
>> + return udp_sendmsg(sk, msg, len);
>> +
>> + /* Do a quick destination sanity check before corking. */
>> + if (sin6) {
>> + if (msg->msg_namelen < offsetof(struct sockaddr, sa_data))
>> + return -EINVAL;
>> + if (sin6->sin6_family == AF_INET6) {
>> + if (msg->msg_namelen < SIN6_LEN_RFC2133)
>> + return -EINVAL;
>> + if (ipv6_addr_any(&sin6->sin6_addr) &&
>> + ipv6_addr_v4mapped(&np->saddr))
>> + return -EINVAL;
>
> It looks like 'any' destination with ipv4 mapped source is now
> rejected, while the existing code accept it.
It should be up->pending == AF_INET6 to get there, and previously it'd
fall into udp_sendmsg() and fail
if (unlikely(up->pending != AF_INET))
return -EINVAL;
I don't see it anyhow rejecting cases that were working before.
Can you elaborate a bit?
--
Pavel Begunkov
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: phy: micrel: Allow probing without .driver_data
From: patchwork-bot+netdevbpf @ 2022-05-16 20:10 UTC (permalink / raw)
To: Fabio Estevam; +Cc: kuba, davem, andrew, netdev, festevam
In-Reply-To: <20220513114613.762810-1-festevam@gmail.com>
Hello:
This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Fri, 13 May 2022 08:46:12 -0300 you wrote:
> From: Fabio Estevam <festevam@denx.de>
>
> Currently, if the .probe element is present in the phy_driver structure
> and the .driver_data is not, a NULL pointer dereference happens.
>
> Allow passing .probe without .driver_data by inserting NULL checks
> for priv->type.
>
> [...]
Here is the summary with links:
- [net-next,1/2] net: phy: micrel: Allow probing without .driver_data
https://git.kernel.org/netdev/net-next/c/f2ef6f7539c6
- [net-next,2/2] net: phy: micrel: Use the kszphy probe/suspend/resume
https://git.kernel.org/netdev/net-next/c/8e6004dfecb7
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] NFC: nci: fix sleep in atomic context bugs caused by nci_skb_alloc
From: Jakub Kicinski @ 2022-05-16 20:07 UTC (permalink / raw)
To: Duoming Zhou
Cc: netdev, krzysztof.kozlowski, linux-kernel, davem, edumazet,
pabeni, gregkh, alexander.deucher, broonie
In-Reply-To: <20220513133355.113222-1-duoming@zju.edu.cn>
On Fri, 13 May 2022 21:33:55 +0800 Duoming Zhou wrote:
> Fixes: 6a2968aaf50c ("NFC: basic NCI protocol implementation ")
> Fixes: 11f54f228643 ("NFC: nci: Add HCI over NCI protocol support")
Are there more bad callers? If st_nci_se_wt_timeout is the only source
of trouble then the fixes tag should point to when it was added, rather
than when the callee was added.
^ permalink raw reply
* Re: [PATCH bpf-next v3] bpftool: Use sysfs vmlinux when dumping BTF by ID
From: Yonghong Song @ 2022-05-16 20:03 UTC (permalink / raw)
To: Larysa Zaremba, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko
Cc: netdev, bpf, linux-kernel, Martin KaFai Lau, Song Liu,
John Fastabend, KP Singh, Quentin Monnet, Maciej Fijalkowski,
Alexander Lobakin
In-Reply-To: <20220513121743.12411-1-larysa.zaremba@intel.com>
On 5/13/22 5:17 AM, Larysa Zaremba wrote:
> Currently, dumping almost all BTFs specified by id requires
> using the -B option to pass the base BTF. For kernel module
> BTFs the vmlinux BTF sysfs path should work.
This is not precise. It should be that
dumping all module BTFs specified by id requires using the -B
option. In current situation, to dump a btf associated with
a bpf program, -B option is not needed.
>
> This patch simplifies dumping by ID usage by loading
> vmlinux BTF from sysfs as base, if base BTF was not specified
> and the ID corresponds to a kernel module BTF.
>
> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> Reviewed-by: Alexander Lobakin <alexandr.lobakin@intel.com>
> ---
> From v2[0]:
> - instead of using vmlinux as base only after the first unsuccessful
> attempt, set base to vmlinux before loading in applicable cases, precisely
> if no base was provided by user and id belongs to a kernel module BTF.
>
> From v1[1]:
> - base BTF is assumed to be vmlinux only for kernel BTFs.
>
> [0] https://lore.kernel.org/bpf/20220505130507.130670-1-larysa.zaremba@intel.com/
> [1] https://lore.kernel.org/bpf/20220428111442.111805-1-larysa.zaremba@intel.com/
> ---
> tools/bpf/bpftool/btf.c | 65 +++++++++++++++++++++++++++++++++++------
> 1 file changed, 56 insertions(+), 9 deletions(-)
>
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index a2c665beda87..0eb105c416fc 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -459,6 +459,54 @@ static int dump_btf_c(const struct btf *btf,
> return err;
> }
>
> +static const char sysfs_vmlinux[] = "/sys/kernel/btf/vmlinux";
> +
> +static struct btf *get_vmlinux_btf_from_sysfs(void)
> +{
> + struct btf *base;
> +
> + base = btf__parse(sysfs_vmlinux, NULL);
> + if (libbpf_get_error(base)) {
> + p_err("failed to parse vmlinux BTF at '%s': %ld\n",
> + sysfs_vmlinux, libbpf_get_error(base));
nit: libbpf_get_error() is called twice. Can be consolidated into
one call.
> + base = NULL;
> + }
> +
> + return base;
> +}
> +
> +#define BTF_NAME_BUFF_LEN 64
> +
> +static bool btf_is_kernel_module(__u32 btf_id)
> +{
> + struct bpf_btf_info btf_info = {};
> + char btf_name[BTF_NAME_BUFF_LEN];
> + int btf_fd;
> + __u32 len;
> + int err;
> +
> + btf_fd = bpf_btf_get_fd_by_id(btf_id);
> + if (btf_fd < 0) {
> + p_err("can't get BTF object by id (%u): %s",
> + btf_id, strerror(errno));
I am not sure whether we want p_err here or not.
The function is to test whether btf is a kernel_module.
If anything wrong happens here. The original logic
follows and an error will be printed out any way.
> + return false;
> + }
> +
> + len = sizeof(btf_info);
> + btf_info.name = ptr_to_u64(btf_name);
> + btf_info.name_len = sizeof(btf_name);
> + err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
> + close(btf_fd);
> +
> + if (err) {
> + p_err("can't get BTF (ID %u) object info: %s",
> + btf_id, strerror(errno));
The same as above, probably we don't need p_err here.
> + return false;
> + }
> +
> + return strncmp(btf_name, "vmlinux", sizeof(btf_name)) && btf_info.kernel_btf;
This should work considering current module names but the code itself
doesn't sound right. The characters after "vmlinux" could be arbitrary.
strncmp(btf_name, "vmlinux", strlen("vmlinux") + 1) is better.
> +}
> +
> static int do_dump(int argc, char **argv)
> {
> struct btf *btf = NULL, *base = NULL;
> @@ -536,18 +584,11 @@ static int do_dump(int argc, char **argv)
> NEXT_ARG();
> } else if (is_prefix(src, "file")) {
> const char sysfs_prefix[] = "/sys/kernel/btf/";
> - const char sysfs_vmlinux[] = "/sys/kernel/btf/vmlinux";
>
> if (!base_btf &&
> strncmp(*argv, sysfs_prefix, sizeof(sysfs_prefix) - 1) == 0 &&
> - strcmp(*argv, sysfs_vmlinux) != 0) {
> - base = btf__parse(sysfs_vmlinux, NULL);
> - if (libbpf_get_error(base)) {
> - p_err("failed to parse vmlinux BTF at '%s': %ld\n",
> - sysfs_vmlinux, libbpf_get_error(base));
> - base = NULL;
> - }
> - }
> + strcmp(*argv, sysfs_vmlinux))
> + base = get_vmlinux_btf_from_sysfs();
>
> btf = btf__parse_split(*argv, base ?: base_btf);
> err = libbpf_get_error(btf);
> @@ -591,6 +632,12 @@ static int do_dump(int argc, char **argv)
> }
>
> if (!btf) {
> + if (!base_btf && btf_is_kernel_module(btf_id)) {
> + p_info("Warning: valid base BTF was not specified with -B option, falling back on standard base BTF (%s)",
> + sysfs_vmlinux);
Having 'Warning' for p_info seems not appropriate. Similar to other
p_info, you can just remove 'Warning: ".
> + base_btf = get_vmlinux_btf_from_sysfs();
> + }
> +
> btf = btf__load_from_kernel_by_id_split(btf_id, base_btf);
> err = libbpf_get_error(btf);
> if (err) {
^ permalink raw reply
* Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock
From: Josua Mayer @ 2022-05-16 19:48 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Michael Walle, alexandru.ardelean, alvaro.karsz, davem, edumazet,
krzysztof.kozlowski+dt, michael.hennerich, netdev, pabeni,
robh+dt
In-Reply-To: <20220516104336.3a76579e@kernel.org>
Am 16.05.22 um 20:43 schrieb Jakub Kicinski:
> On Sun, 15 May 2022 10:16:47 +0300 Josua Mayer wrote:
>> Am 13.05.22 um 01:44 schrieb Jakub Kicinski:
>>> On Thu, 12 May 2022 23:20:18 +0200 Michael Walle wrote:
>>>>> It's pure speculation on my side. I don't even know if PHYs use
>>>>> the recovered clock to clock its output towards the MAC or that's
>>>>> a different clock domain.
>>>>>
>>>>> My concern is that people will start to use DT to configure SyncE which
>>>>> is entirely a runtime-controllable thing, and doesn't belong.
>> Okay.
>> However phy drivers do not seem to implement runtime control of those
>> clock output pins currently, so they are configured once in DT.
> To me that means nobody needs the recovered clock.
Doesn't need it, or is overwhelmed by the idea of figuring out how to
implement it properly.
>>>>> Hence
>>>>> my preference to hide the recovered vs free-running detail if we can
>>>>> pick one that makes most sense for now.
>> I am not a fan of hiding information. The clock configuration register
>> clearly supports this distinction.
> Unless you expose all registers as a direct API to the user you'll be
> "hiding information". I don't think we are exposing all possible
> registers for this PHY, the two bits in question are no different.
>
>> Is this a political stance to say users may not "accidentally" enable
>> SyncE by patching DT?
>> If so we should print a warning message when someone selects it?
> Why would we add a feature and then print a warning? We can always add
> the support later, once we have a use case for it.
I would not call it a feature.
We can e.g. not print a warning, and instead put in the DT binding a
note that the recovered variants are for SyncE which Linux does not
support.
As to why we would add the -recovered options,
for starters this allows curious developers to search for the term to
get an idea which PHYs would technically support it.
That it would also allow tinkering with SyncE to me is a plus, but for
you clearly a minus, and I can not make a strong case.
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.
> I see. That makes sense, but then wouldn't it make more sense to pick
> the (simple) free-running one? As for SyncE you'd need the recovered
> clock.
>>> Sounds good.
>> Yep, it seems recovered clock is only for SyncE - and only if there is a
>> master clock on the network. Sadly however documentation is sparse and I
>> do not know if the adi phys would fall back to using their internal
>> clock, or just refuse to operate at all.
^ permalink raw reply
* Re: [RFC PATCH bpf-next v2 0/7] bpf: rstat: cgroup hierarchical stats
From: Tejun Heo @ 2022-05-16 19:39 UTC (permalink / raw)
To: Yosry Ahmed
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Hao Luo, Zefan Li, Johannes Weiner, Shuah Khan,
Roman Gushchin, Michal Hocko, Stanislav Fomichev, David Rientjes,
Greg Thelen, Shakeel Butt, linux-kernel, netdev, bpf, cgroups
In-Reply-To: <20220515023504.1823463-1-yosryahmed@google.com>
Hello,
Looks good from cgroup side.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH net-next v2] docs: ctucanfd: Use 'kernel-figure' directive instead of 'figure'
From: Pavel Pisa @ 2022-05-16 19:31 UTC (permalink / raw)
To: Akira Yokosawa
Cc: Marc Kleine-Budde, netdev, David S. Miller, Jakub Kicinski,
Martin Jerabek, Ondrej Ille, linux-doc, linux-kernel
In-Reply-To: <38d102ab-d0b6-3467-4dce-4a9d4aa9e39d@gmail.com>
Dear Akira,
On Monday 16 of May 2022 13:24:49 Akira Yokosawa wrote:
> On Wed, 11 May 2022 08:45:43 +0900, Akira Yokosawa wrote:
> > Two issues were observed in the ReST doc added by commit c3a0addefbde
> > ("docs: ctucanfd: CTU CAN FD open-source IP core documentation.")
> > with Sphinx versions 2.4.4 and 4.5.0.
> >
> > The plain "figure" directive broke "make pdfdocs" due to a missing
> > PDF figure. For conversion of SVG -> PDF to work, the "kernel-figure"
> > directive, which is an extension for kernel documentation, should
> > be used instead.
> >
> > The directive of "code:: raw" causes a warning from both
> > "make htmldocs" and "make pdfdocs", which reads:
> >
> > [...]/can/ctu/ctucanfd-driver.rst:75: WARNING: Pygments lexer name
> > 'raw' is not known
> >
> > A plain literal-block marker should suffice where no syntax
> > highlighting is intended.
> >
> > Fix the issues by using suitable directive and marker.
> >
> > Signed-off-by: Akira Yokosawa <akiyks@gmail.com>
> > Fixes: c3a0addefbde ("docs: ctucanfd: CTU CAN FD open-source IP core
> > documentation.") Acked-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> > Cc: Martin Jerabek <martin.jerabek01@gmail.com>
> > Cc: Ondrej Ille <ondrej.ille@gmail.com>
> > Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> > ---
> > Changes in v1 -> v2
> > - no change in diff
> > - added explicit Sphinx versions the issues were observed
> > - picked Pavel's Acked-by
>
> Gentle ping to netdev maintainers.
>
> I believe this one should go upstream together with the
> offending commit.
I think that the patch is on the right route thanks
to Marc Kleine-Budde already, it is in the linux-can-next
testing
https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?h=testing&id=f898bbb9c92e33dcbe7ee29b8861b707c2cd509e
I hope that it would reach net-next after next
linux-can-next merge.
Best wishes,
Pavel
--
Pavel Pisa
phone: +420 603531357
e-mail: pisa@cmp.felk.cvut.cz
Department of Control Engineering FEE CVUT
Karlovo namesti 13, 121 35, Prague 2
university: http://control.fel.cvut.cz/
personal: http://cmp.felk.cvut.cz/~pisa
projects: https://www.openhub.net/accounts/ppisa
CAN related:http://canbus.pages.fel.cvut.cz/
Open Technologies Research Education and Exchange Services
https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home
^ permalink raw reply
* Re: [PATCH] net: ax88179: add proper error handling of usb read errors
From: Jakub Kicinski @ 2022-05-16 19:31 UTC (permalink / raw)
To: David Kahurani
Cc: netdev, syzbot+d3dbdf31fbe9d8f5f311, davem, jgg, linux-kernel,
linux-usb, phil, syzkaller-bugs, arnd, dan.carpenter
In-Reply-To: <20220514133234.33796-1-k.kahurani@gmail.com>
On Sat, 14 May 2022 16:32:34 +0300 David Kahurani wrote:
> - ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> - 1, 1, &buf);
> + ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> + 1, 1, &buf);
> + if (ret) {
> + netdev_dbg(dev->net,
> + "Failed to read SROM_CMD: %d\n",
> + ret);
> + return ret;
> + }
>
> if (time_after(jiffies, jtimeout))
> return -EINVAL;
>
> } while (buf & EEP_BUSY);
I agree with Pavel, this seems unnecessarily strict. If the error is
not ENODEV we can keep looping.
> @@ -1581,7 +1731,13 @@ static int ax88179_link_reset(struct usbnet *dev)
> &ax179_data->rxctl);
>
> /*link up, check the usb device control TX FIFO full or empty*/
> - ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
> + ret = ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
> +
> + if (ret) {
> + netdev_dbg(dev->net, "Failed to read TX FIFO: %d\n",
> + ret);
> + return ret;
> + }
Please don't add any empty lines within the error checking.
Empty lines are supposed to separate logically separate blocks of code.
Error checking is very much logically part of the call. And no empty
line betwee netdev_dbg() and return ret; either. In this submission you
have all possible configurations of having or not having empty lines
before the if or before the return. None of them should be there, and
please be consistent.
^ permalink raw reply
* Re: [RFC PATCH 2/3] dt-bindings: can: ctucanfd: add properties for HW timestamping
From: Pavel Pisa @ 2022-05-16 19:21 UTC (permalink / raw)
To: Marc Kleine-Budde, Rob Herring
Cc: Matej Vasilevski, linux-can, devicetree, netdev, ondrej.ille,
martin.jerabek01
In-Reply-To: <20220516163445.qxz3xlohuquqwbwl@pengutronix.de>
Hello Rob and Marc,
thanks for comment and help.
This patch is marked as RFC and not intended for direct
application. We plan to gather feetback, adjust code
and probably even IP core HDL based on suggestions,
then we plan more testing and when we will be ready and
time allows, new version with plea for merge will provided.
On Monday 16 of May 2022 18:34:45 Marc Kleine-Budde wrote:
> On 16.05.2022 11:02:50, Rob Herring wrote:
> > On Fri, May 13, 2022 at 01:27:06AM +0200, Matej Vasilevski wrote:
> > > Extend dt-bindings for CTU CAN-FD IP core with necessary properties
> > > to enable HW timestamping for platform devices. Since the timestamping
> > > counter is provided by the system integrator usign those IP cores in
> > > their FPGA design, we need to have the properties specified in device
> > > tree.
> > >
> > > Signed-off-by: Matej Vasilevski <matej.vasilevski@seznam.cz>
> > > ---
> > > .../bindings/net/can/ctu,ctucanfd.yaml | 34 +++++++++++++++++--
> > > 1 file changed, 31 insertions(+), 3 deletions(-)
> >
> > What's the base for this patch? Doesn't apply for me.
It is based on the series of complete CTU CAN FD support
which has been accepted into net-next.
The DTC part has gone through your review and has been
ACKed longer time ago. We have spent considerable time
to resolve suggested driver changes - headers files generation
from HDL tool chain, etc.
We inline to version when most of the info will be directly
provided by the core HW except for optional second clocks
probably.
Best wishes,
Pavel
> > > diff --git
> > > a/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml
> > > b/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml index
> > > fb34d971dcb3..c3693dadbcd8 100644
> > > --- a/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml
> > > +++ b/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.yaml
> > > @@ -41,9 +41,35 @@ properties:
> > >
> > > clocks:
> > > description: |
> > > - phandle of reference clock (100 MHz is appropriate
> > > - for FPGA implementation on Zynq-7000 system).
> > > + Phandle of reference clock (100 MHz is appropriate for FPGA
> > > + implementation on Zynq-7000 system). If you wish to use
> > > timestamps + from the core, add a second phandle with the clock
> > > used for timestamping + (can be the same as the first clock).
> > > + maxItems: 2
> >
> > With more than 1, you have to define what each entry is. IOW, use
> > 'items'.
> >
> > > +
> > > + clock-names:
> > > + description: |
> > > + Specify clock names for the "clocks" property. The first clock
> > > name + doesn't matter, the second has to be "ts_clk". Timestamping
> > > frequency + is then obtained from the "ts_clk" clock. This takes
> > > precedence over + the ts-frequency property.
> > > + You can omit this property if you don't need timestamps.
> > > + maxItems: 2
> >
> > You must define what the names are as a schema.
> >
> > > +
> > > + ts-used-bits:
> > > + description: width of the timestamping counter
> > > + maxItems: 1
> > > + items:
> >
> > Not an array, so you don't need maxItems nor items.
> >
> > > + minimum: 8
> > > + maximum: 64
> > > +
> > > + ts-frequency:
> >
> > Use a standard unit suffix.
> >
> > > + description: |
> > > + Frequency of the timestamping counter. Set this if you want to
> > > get + timestamps, but you didn't set the timestamping clock in
> > > clocks property. maxItems: 1
> > > + items:
> >
> > Not an array.
> >
> >
> > Is timestamping a common feature for CAN or is this specific to this
> > controller? In the latter case, you need vendor prefixes on these
> > properties. In the former case, you need to define them in a common
> > schema.
>
> This property describes the usable with of the free running timer and
> the timestamps generated by it. This is similar to the free running
> timer and time stamps as found on PTP capable Ethernet NICs. But the
> ctucanfd comes in a hardware description language that can be
> parametrized and synthesized into your own FPGA.
>
> To answer your question, timestamping is common in newer CAN cores, but
> the width of the timestamping register is usually fixed and thus hard
> coded in the driver.
>
> regards,
> Marc
^ permalink raw reply
* Re: [PATCH net-next v2 2/5] net: dsa: add out-of-band tagging protocol
From: Jakub Kicinski @ 2022-05-16 19:20 UTC (permalink / raw)
To: Maxime Chevallier
Cc: davem, Rob Herring, netdev, linux-kernel, devicetree,
thomas.petazzoni, Andrew Lunn, Florian Fainelli, Heiner Kallweit,
Russell King, linux-arm-kernel, Vladimir Oltean, Luka Perkov,
Robert Marko
In-Reply-To: <20220514150656.122108-3-maxime.chevallier@bootlin.com>
On Sat, 14 May 2022 17:06:53 +0200 Maxime Chevallier wrote:
> This tagging protocol is designed for the situation where the link
> between the MAC and the Switch is designed such that the Destination
> Port, which is usually embedded in some part of the Ethernet Header, is
> sent out-of-band, and isn't present at all in the Ethernet frame.
>
> This can happen when the MAC and Switch are tightly integrated on an
> SoC, as is the case with the Qualcomm IPQ4019 for example, where the DSA
> tag is inserted directly into the DMA descriptors. In that case,
> the MAC driver is responsible for sending the tag to the switch using
> the out-of-band medium. To do so, the MAC driver needs to have the
> information of the destination port for that skb.
>
> This out-of-band tagging protocol is using the very beggining of the skb
> headroom to store the tag. The drawback of this approch is that the
> headroom isn't initialized upon allocating it, therefore we have a
> chance that the garbage data that lies there at allocation time actually
> ressembles a valid oob tag. This is only problematic if we are
> sending/receiving traffic on the master port, which isn't a valid DSA
> use-case from the beggining. When dealing from traffic to/from a slave
> port, then the oob tag will be initialized properly by the tagger or the
> mac driver through the use of the dsa_oob_tag_push() call.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
This must had been asked on v1 but there's no trace of it in the
current submission afaict...
If the tag is passed in the descriptor how is this not a pure switchdev
driver? The explanation must be preserved somehow.
^ permalink raw reply
* Re: [linux-next:master] BUILD REGRESSION 1e1b28b936aed946122b4e0991e7144fdbbfd77e
From: Jakub Kicinski @ 2022-05-16 19:06 UTC (permalink / raw)
To: kernel test robot
Cc: Andrew Morton, netdev, linux-staging, linux-sh, linux-omap,
linux-mm, linux-mips, linux-kselftest, linux-hwmon, linux-fbdev,
linux-arm-msm, linux-arm-kernel, kvm, kunit-dev, intel-gfx,
dri-devel, bpf, amd-gfx
In-Reply-To: <6280f965.kTCPpIEVY9TwoNre%lkp@intel.com>
On Sun, 15 May 2022 21:00:21 +0800 kernel test robot wrote:
> tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> branch HEAD: 1e1b28b936aed946122b4e0991e7144fdbbfd77e Add linux-next specific files for 20220513
>
> Error/Warning reports:
>
> https://lore.kernel.org/linux-mm/202204181931.klAC6fWo-lkp@intel.com
> https://lore.kernel.org/linux-mm/202204291924.vTGZmerI-lkp@intel.com
> https://lore.kernel.org/linux-mm/202205031017.4TwMan3l-lkp@intel.com
> https://lore.kernel.org/linux-mm/202205041248.WgCwPcEV-lkp@intel.com
> https://lore.kernel.org/linux-mm/202205122113.uLKzd3SZ-lkp@intel.com
> https://lore.kernel.org/linux-mm/202205150051.3RzuooAG-lkp@intel.com
> https://lore.kernel.org/linux-mm/202205150117.sd6HzBVm-lkp@intel.com
> https://lore.kernel.org/lkml/202205100617.5UUm3Uet-lkp@intel.com
> https://lore.kernel.org/llvm/202204210555.DNvfHvIb-lkp@intel.com
> https://lore.kernel.org/llvm/202205060132.uhqyUx1l-lkp@intel.com
> https://lore.kernel.org/llvm/202205120010.zWBednzM-lkp@intel.com
> https://lore.kernel.org/llvm/202205141122.qihFGUem-lkp@intel.com
>
> Error/Warning: (recently discovered and may have been fixed)
>
> <command-line>: fatal error: ./include/generated/utsrelease.h: No such file or directory
> arch/arm/mach-versatile/versatile.c:56:14: warning: no previous prototype for function 'mmc_status' [-Wmissing-prototypes]
> arch/x86/kvm/pmu.h:20:32: warning: 'vmx_icl_pebs_cpu' defined but not used [-Wunused-const-variable=]
> drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dp.c:5102:7: warning: variable 'allow_lttpr_non_transparent_mode' set but not used [-Wunused-but-set-variable]
> drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dp.c:5147:6: warning: no previous prototype for function 'dp_parse_lttpr_mode' [-Wmissing-prototypes]
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1364:5: warning: no previous prototype for 'amdgpu_discovery_get_mall_info' [-Wmissing-prototypes]
> drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c:1983:6: warning: no previous prototype for function 'gfx_v11_0_rlc_stop' [-Wmissing-prototypes]
> drivers/gpu/drm/amd/amdgpu/soc21.c:171:6: warning: no previous prototype for 'soc21_grbm_select' [-Wmissing-prototypes]
> drivers/gpu/drm/solomon/ssd130x-spi.c:154:35: warning: 'ssd130x_spi_table' defined but not used [-Wunused-const-variable=]
> drivers/hwmon/nct6775-platform.c:199:9: sparse: unsigned char
> drivers/hwmon/nct6775-platform.c:199:9: sparse: void
> drivers/video/fbdev/omap/hwa742.c:492:5: warning: no previous prototype for 'hwa742_update_window_async' [-Wmissing-prototypes]
> fs/buffer.c:2254:5: warning: stack frame size (2144) exceeds limit (1024) in 'block_read_full_folio' [-Wframe-larger-than]
> fs/ntfs/aops.c:378:12: warning: stack frame size (2224) exceeds limit (1024) in 'ntfs_read_folio' [-Wframe-larger-than]
> kernel/trace/fgraph.c:37:12: warning: no previous prototype for 'ftrace_enable_ftrace_graph_caller' [-Wmissing-prototypes]
> kernel/trace/fgraph.c:46:12: warning: no previous prototype for 'ftrace_disable_ftrace_graph_caller' [-Wmissing-prototypes]
Is this report CCed everywhere or there's a reason why netdev@ is CCed?
I'm trying to figure out we need to care and it's not obvious..
^ permalink raw reply
* Re: [PATCH net-next 4/4] net: call skb_defer_free_flush() before each napi_poll()
From: Jakub Kicinski @ 2022-05-16 18:56 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Eric Dumazet, David S . Miller, Paolo Abeni, netdev
In-Reply-To: <CANn89iL9xw83hEGA4=K-F1qkjyRhvAJ85c9W5nY1Fsmq777V0A@mail.gmail.com>
On Mon, 16 May 2022 11:26:14 -0700 Eric Dumazet wrote:
> On Mon, May 16, 2022 at 11:21 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Sun, 15 May 2022 21:24:56 -0700 Eric Dumazet wrote:
> > > -end:
> > > - skb_defer_free_flush(sd);
> > > +end:;
> >
> > Sorry for the nit pick but can I remove this and just return like we
> > did before f3412b3879b4? Is there a reason such "label:;}" is good?
>
> I thought that having a return in the middle of this function would
> hurt us at some point.
I guess personal preference. Let's leave it unless someone else shares
my disregard for pointlessly jumping to the closing bracket :)
^ permalink raw reply
* Re: [PATCH net-next 1/4] net: fix possible race in skb_attempt_defer_free()
From: Jakub Kicinski @ 2022-05-16 18:54 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Eric Dumazet, David S . Miller, Paolo Abeni, netdev
In-Reply-To: <CANn89iJnS5Yyofudjbr7ZO5okRF67w1FRebQ71h3Bg75CA_L+Q@mail.gmail.com>
On Mon, 16 May 2022 11:24:40 -0700 Eric Dumazet wrote:
> On Mon, May 16, 2022 at 11:16 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > If I'm reading this right this is useful for backports but in net-next
> > it really is a noop? The -EBUSY would be perfectly safe to ignore?
> > Just checking.
>
> Not sure I understand the question.
>
> trigger_rx_softirq() and friends were only in net-next, so there is no
> backport needed.
>
> Are you talking of calls from net_rps_send_ipi() ?
> These are fine, because we own an atomic bit (NAPI_STATE_SCHED).
Ah, I think I get it now. It was unclear what's the problem this patch
is solving this part of the commit message really is key:
> This is a common issue with smp_call_function_single_async().
> Callers must ensure correct synchronization and serialization.
smp_call_function_single_async() does not protect its own internal state
so we need to wrap it in our own locking (of some form thereof).
Sorry for the noise.
^ permalink raw reply
* Re: [net-next PATCH V2] octeontx2-pf: Add support for adaptive interrupt coalescing
From: Jakub Kicinski @ 2022-05-16 18:28 UTC (permalink / raw)
To: Suman Ghosh
Cc: davem, edumazet, pabeni, sgoutham, sbhatta, gakula, Sunil.Goutham,
hkelam, colin.king, netdev
In-Reply-To: <20220516105359.746919-1-sumang@marvell.com>
On Mon, 16 May 2022 16:23:59 +0530 Suman Ghosh wrote:
> @@ -1230,7 +1262,7 @@ static int otx2_set_link_ksettings(struct net_device *netdev,
>
> static const struct ethtool_ops otx2_ethtool_ops = {
> .supported_coalesce_params = ETHTOOL_COALESCE_USECS |
> - ETHTOOL_COALESCE_MAX_FRAMES,
> + ETHTOOL_COALESCE_MAX_FRAMES | ETHTOOL_COALESCE_USE_ADAPTIVE,
Ah there it is. Now you actually tested it with upstream :/
Please wrap the line.
Also please reject user trying to set asymmetric adaptive mode since
you don't seem to support it:
> + /* Check and update coalesce status */
> + if ((pfvf->flags & OTX2_FLAG_ADPTV_INT_COAL_ENABLED) ==
> + OTX2_FLAG_ADPTV_INT_COAL_ENABLED) {
> + priv_coalesce_status = 1;
> + if (!ec->use_adaptive_rx_coalesce || !ec->use_adaptive_tx_coalesce)
> + pfvf->flags &= ~OTX2_FLAG_ADPTV_INT_COAL_ENABLED;
> + } else {
> + priv_coalesce_status = 0;
> + if (ec->use_adaptive_rx_coalesce || ec->use_adaptive_tx_coalesce)
> + pfvf->flags |= OTX2_FLAG_ADPTV_INT_COAL_ENABLED;
If I'm reading this right user doing:
ethtool -C eth0 adaptive-rx on adaptive-tx off
multiple times will keep switching between adaptive and non-adaptive
mode. This will be super confusing to automation which may assume
configuration commands are idempotent.
^ permalink raw reply
* Re: [PATCH v5 04/15] landlock: helper functions refactoring
From: Mickaël Salaün @ 2022-05-16 18:28 UTC (permalink / raw)
To: Konstantin Meskhidze
Cc: willemdebruijn.kernel, linux-security-module, netdev,
netfilter-devel, yusongping, anton.sirazetdinov
In-Reply-To: <8c272564-1033-f100-23b6-db6579085fd0@huawei.com>
On 16/05/2022 19:43, Konstantin Meskhidze wrote:
>
>
> 5/16/2022 8:14 PM, Mickaël Salaün пишет:
>>
>> On 16/05/2022 17:20, Konstantin Meskhidze wrote:
>>> Unmask_layers(), init_layer_masks() and
>>> get_handled_accesses() helper functions move to
>>> ruleset.c and rule_type argument is added.
>>> This modification supports implementing new rule
>>> types into next landlock versions.
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
[...]
>>> -/*
>>> - * @layer_masks is read and may be updated according to the access
>>> request and
>>> - * the matching rule.
>>> - *
>>> - * Returns true if the request is allowed (i.e. relevant layer masks
>>> for the
>>> - * request are empty).
>>> - */
>>> -static inline bool
>>> -unmask_layers(const struct landlock_rule *const rule,
>>> - const access_mask_t access_request,
>>> - layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
>>
>> Moving these entire blocks of code make the review/diff impossible.
>> Why moving these helpers?
>
> Cause these helpers are going to be used both for filesystem and
> network. I moved them into ruleset.c/h
Right. Please create a commit which only moves these helpers without
modifying them, and explain in the commit message that this removes
inlined code. We'll see later if this adds a visible performance impact.
[...]
>>> @@ -519,17 +413,25 @@ static int check_access_path_dual(
>>>
>>> if (unlikely(dentry_child1)) {
>>> unmask_layers(find_rule(domain, dentry_child1),
>>> - init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS,
>>> - &_layer_masks_child1),
>>> - &_layer_masks_child1);
>>> + init_layer_masks(domain,
>>> + LANDLOCK_MASK_ACCESS_FS,
>>> + &_layer_masks_child1,
>>> + sizeof(_layer_masks_child1),
>>> + LANDLOCK_RULE_PATH_BENEATH),
>>> + &_layer_masks_child1,
>>> + ARRAY_SIZE(_layer_masks_child1));
>>
>> There is a lot of formatting diff and that makes the review difficult.
>> Please format everything with clang-format-14.
>
> Ok. Do you have some tool that helps you with editing code with clang
> format?
I just run `clang-format-14 -i` on files before each commit. Some
editors such as VSCode can handle the clang-format configuration (which
is in the Linux source tree).
>>
>>> layer_masks_child1 = &_layer_masks_child1;
>>> child1_is_directory = d_is_dir(dentry_child1);
>>> }
>>> if (unlikely(dentry_child2)) {
>>> unmask_layers(find_rule(domain, dentry_child2),
>>> - init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS,
>>> - &_layer_masks_child2),
>>> - &_layer_masks_child2);
>>> + init_layer_masks(domain,
>>> + LANDLOCK_MASK_ACCESS_FS,
>>> + &_layer_masks_child2,
>>> + sizeof(_layer_masks_child2),
>>> + LANDLOCK_RULE_PATH_BENEATH),
>>> + &_layer_masks_child2,
>>> + ARRAY_SIZE(_layer_masks_child2));
>>> layer_masks_child2 = &_layer_masks_child2;
>>> child2_is_directory = d_is_dir(dentry_child2);
>>> }
>>> @@ -582,14 +484,15 @@ static int check_access_path_dual(
>>>
>>> rule = find_rule(domain, walker_path.dentry);
>>> allowed_parent1 = unmask_layers(rule, access_masked_parent1,
>>> - layer_masks_parent1);
>>> + layer_masks_parent1,
>>> + ARRAY_SIZE(*layer_masks_parent1));
>>> allowed_parent2 = unmask_layers(rule, access_masked_parent2,
>>> - layer_masks_parent2);
>>> + layer_masks_parent2,
>>> + ARRAY_SIZE(*layer_masks_parent2));
>>>
>>> /* Stops when a rule from each layer grants access. */
>>> if (allowed_parent1 && allowed_parent2)
>>> break;
>>> -
>>
>> There is no place for such formatting/whitespace patches.
>>
> I missed that. scripts/checkpatch.pl did not show any problem here.
checkpatch.pl doesn't warn about whitespace changes.
> I will fix it. Thanks.
>>
>>> jump_up:
>>> if (walker_path.dentry == walker_path.mnt->mnt_root) {
>>> if (follow_up(&walker_path)) {
>>> @@ -645,7 +548,9 @@ static inline int check_access_path(const struct
>>> landlock_ruleset *const domain,
>>> {
>>> layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
>>>
>>> - access_request = init_layer_masks(domain, access_request,
>>> &layer_masks);
>>> + access_request = init_layer_masks(domain, access_request,
>>> + &layer_masks, sizeof(layer_masks),
>>> + LANDLOCK_RULE_PATH_BENEATH);
>>> return check_access_path_dual(domain, path, access_request,
>>> &layer_masks, NULL, 0, NULL, NULL);
>>> }
>>> @@ -729,7 +634,8 @@ static bool collect_domain_accesses(
>>> return true;
>>>
>>> access_dom = init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS,
>>> - layer_masks_dom);
>>> + layer_masks_dom, sizeof(*layer_masks_dom),
>>> + LANDLOCK_RULE_PATH_BENEATH);
>>>
>>> dget(dir);
>>> while (true) {
>>> @@ -737,7 +643,8 @@ static bool collect_domain_accesses(
>>>
>>> /* Gets all layers allowing all domain accesses. */
>>> if (unmask_layers(find_rule(domain, dir), access_dom,
>>> - layer_masks_dom)) {
>>> + layer_masks_dom,
>>> + ARRAY_SIZE(*layer_masks_dom))) {
>>> /*
>>> * Stops when all handled accesses are allowed by at
>>> * least one rule in each layer.
>>> @@ -851,9 +758,10 @@ static int current_check_refer_path(struct
>>> dentry *const old_dentry,
>>> * The LANDLOCK_ACCESS_FS_REFER access right is not required
>>> * for same-directory referer (i.e. no reparenting).
>>> */
>>> - access_request_parent1 = init_layer_masks(
>>> - dom, access_request_parent1 | access_request_parent2,
>>> - &layer_masks_parent1);
>>> + access_request_parent1 = init_layer_masks(dom,
>>> + access_request_parent1 | access_request_parent2,
>>> + &layer_masks_parent1, sizeof(layer_masks_parent1),
>>> + LANDLOCK_RULE_PATH_BENEATH);
>>> return check_access_path_dual(dom, new_dir,
>>> access_request_parent1,
>>> &layer_masks_parent1, NULL, 0,
>>> @@ -861,7 +769,9 @@ static int current_check_refer_path(struct dentry
>>> *const old_dentry,
>>> }
>>>
>>> /* Backward compatibility: no reparenting support. */
>>> - if (!(get_handled_accesses(dom) & LANDLOCK_ACCESS_FS_REFER))
>>> + if (!(get_handled_accesses(dom, LANDLOCK_RULE_PATH_BENEATH,
>>> + LANDLOCK_NUM_ACCESS_FS) &
>>> + LANDLOCK_ACCESS_FS_REFER))
>>> return -EXDEV;
>>>
>>> access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER;
>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>> index 4b4c9953bb32..c4ed783d655b 100644
>>> --- a/security/landlock/ruleset.c
>>> +++ b/security/landlock/ruleset.c
>>> @@ -233,7 +233,8 @@ static int insert_rule(struct landlock_ruleset
>>> *const ruleset,
>>> &(*layers)[0]);
>>> if (IS_ERR(new_rule))
>>> return PTR_ERR(new_rule);
>>> - rb_replace_node(&this->node, &new_rule->node,
>>> &ruleset->root_inode);
>>> + rb_replace_node(&this->node, &new_rule->node,
>>> + &ruleset->root_inode);
>>
>> This is a pure formatting hunk. :/
>>
> Thats strange, cause in my editor I have normal aligment of arguments.
> Could please share clang-format-14 tab size and other format
> parameters?
They are in the .clang-format file. It would be much easier to just run
clang-format-14 -i on your changed files. I guess you had different
changes between consecutive commits.
^ permalink raw reply
* [PATCH bpf 4/4] bpf_trace: pass array of u64 values in kprobe_multi.addrs
From: Eugene Syromiatnikov @ 2022-05-16 18:27 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 | 2 +-
6 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 5b0cf54..86a5544 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2414,7 +2414,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;
@@ -2447,9 +2447,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..b58e2b0 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);
}
--
2.1.4
^ permalink raw reply related
* [PATCH bpf 3/4] bpf_trace: handle compat in kprobe_multi_resolve_syms
From: Eugene Syromiatnikov @ 2022-05-16 18:27 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 | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index d228440..5b0cf54 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2352,16 +2352,21 @@ static int
kprobe_multi_resolve_syms(const void __user *usyms, u32 cnt,
unsigned long *addrs)
{
- unsigned long addr, size;
+ unsigned long addr;
+ size_t sym_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, 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;
@@ -2375,7 +2380,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)
@@ -2384,9 +2392,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;
--
2.1.4
^ permalink raw reply related
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