* Re: [PATCH net 02/15] ibmvnic: process HMC disable command
From: Jakub Kicinski @ 2020-11-21 23:38 UTC (permalink / raw)
To: Lijun Pan; +Cc: netdev, sukadev, drt
In-Reply-To: <20201121153637.17a91ac4@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Sat, 21 Nov 2020 15:36:37 -0800 Jakub Kicinski wrote:
> On Fri, 20 Nov 2020 16:40:36 -0600 Lijun Pan wrote:
> > From: Dany Madden <drt@linux.ibm.com>
> >
> > Currently ibmvnic does not support the disable vnic command from the
> > Hardware Management Console. This patch enables ibmvnic to process
> > CRQ message 0x07, disable vnic adapter.
>
> What user-visible problem does this one solve?
Re-reading the commit message - is Hardware Management Console operated
by a human? So this is basically adding a missing feature, not fixes a
bug? Unless not being able to disable vnic is causing other things to
break.
^ permalink raw reply
* Re: [PATCH net 02/15] ibmvnic: process HMC disable command
From: Jakub Kicinski @ 2020-11-21 23:36 UTC (permalink / raw)
To: Lijun Pan; +Cc: netdev, sukadev, drt
In-Reply-To: <20201120224049.46933-3-ljp@linux.ibm.com>
On Fri, 20 Nov 2020 16:40:36 -0600 Lijun Pan wrote:
> From: Dany Madden <drt@linux.ibm.com>
>
> Currently ibmvnic does not support the disable vnic command from the
> Hardware Management Console. This patch enables ibmvnic to process
> CRQ message 0x07, disable vnic adapter.
What user-visible problem does this one solve?
^ permalink raw reply
* Re: [PATCH net 04/15] ibmvnic: remove free_all_rwi function
From: Jakub Kicinski @ 2020-11-21 23:39 UTC (permalink / raw)
To: Lijun Pan; +Cc: netdev, sukadev, drt
In-Reply-To: <20201120224049.46933-5-ljp@linux.ibm.com>
On Fri, 20 Nov 2020 16:40:38 -0600 Lijun Pan wrote:
> From: Dany Madden <drt@linux.ibm.com>
>
> Remove free_all_rwi() since it is no longer used. (__ibmvnic_remove() was
> the last user of free_all_rwi()).
Squash this with the appropriate change, please.
^ permalink raw reply
* Re: [PATCH] tun: honor IOCB_NOWAIT flag
From: Jakub Kicinski @ 2020-11-21 23:19 UTC (permalink / raw)
To: Jens Axboe; +Cc: Networking
In-Reply-To: <e9451860-96cc-c7c7-47b8-fe42cadd5f4c@kernel.dk>
On Fri, 20 Nov 2020 07:59:54 -0700 Jens Axboe wrote:
> tun only checks the file O_NONBLOCK flag, but it should also be checking
> the iocb IOCB_NOWAIT flag. Any fops using ->read/write_iter() should check
> both, otherwise it breaks users that correctly expect O_NONBLOCK semantics
> if IOCB_NOWAIT is set.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH net 01/15] ibmvnic: handle inconsistent login with reset
From: Jakub Kicinski @ 2020-11-21 23:36 UTC (permalink / raw)
To: Lijun Pan; +Cc: netdev, sukadev, drt
In-Reply-To: <20201120224049.46933-2-ljp@linux.ibm.com>
On Fri, 20 Nov 2020 16:40:35 -0600 Lijun Pan wrote:
> From: Dany Madden <drt@linux.ibm.com>
>
> Inconsistent login with the vnicserver is causing the device to be
> removed. This does not give the device a chance to recover from error
> state. This patch schedules a FATAL reset instead to bring the adapter
> up.
>
> Signed-off-by: Dany Madden <drt@linux.ibm.com>
> Signed-off-by: Lijun Pan <ljp@linux.ibm.com>
Please provide fixes tags for all the patches.
^ permalink raw reply
* Re: pull-request: can-next 2020-11-20
From: Jakub Kicinski @ 2020-11-21 23:09 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: netdev, davem, linux-can, kernel
In-Reply-To: <20201120133318.3428231-1-mkl@pengutronix.de>
On Fri, 20 Nov 2020 14:32:53 +0100 Marc Kleine-Budde wrote:
> The first patch is by Yegor Yefremov and he improves the j1939 documentaton by
> adding tables for the CAN identifier and its fields.
>
> Then there are 8 patches by Oliver Hartkopp targeting the CAN driver
> infrastructure and drivers. These add support for optional DLC element to the
> Classical CAN frame structure. See patch ea7800565a12 ("can: add optional DLC
> element to Classical CAN frame structure") for details. Oliver's last patch
> adds len8_dlc support to several drivers. Stefan Mätje provides a patch to add
> len8_dlc support to the esd_usb2 driver.
>
> The next patch is by Oliver Hartkopp, too and adds support for modification of
> Classical CAN DLCs to CAN GW sockets.
>
> The next 3 patches target the nxp,flexcan DT bindings. One patch by my adds the
> missing uint32 reference to the clock-frequency property. Joakim Zhang's
> patches fix the fsl,clk-source property and add the IMX_SC_R_CAN() macro to the
> imx firmware header file, which will be used in the flexcan driver later.
>
> Another patch by Joakim Zhang prepares the flexcan driver for SCU based
> stop-mode, by giving the existing, GPR based stop-mode, a _GPR postfix.
>
> The next 5 patches are by me, target the flexcan driver, and clean up the
> .ndo_open and .ndo_stop callbacks. These patches try to fix a sporadically
> hanging flexcan_close() during simultanious ifdown, sending of CAN messages and
> probably open CAN bus. I was never aber to reproduce, but these seem to fix the
> problem at the reporting user. As these changes are rather big, I'd like to
> mainline them via net-next/master.
>
> The next patches are by Jimmy Assarsson and Christer Beskow, they add support
> for new USB devices to the existing kvaser_usb driver.
>
> The last patch is by Kaixu Xia and simplifies the return in the
> mcp251xfd_chip_softreset() function in the mcp251xfd driver.
Great, this one finally got into patchwork correctly!
Pulled, thank you!
^ permalink raw reply
* Re: [PATCH 072/141] can: peak_usb: Fix fall-through warnings for Clang
From: Marc Kleine-Budde @ 2020-11-21 23:04 UTC (permalink / raw)
To: Joe Perches, Gustavo A. R. Silva, Wolfgang Grandegger,
David S. Miller, Jakub Kicinski
Cc: linux-can, netdev, linux-kernel, linux-hardening
In-Reply-To: <de5b16cf3fdac1f783e291acc325b78368653ec5.camel@perches.com>
[-- Attachment #1.1: Type: text/plain, Size: 1580 bytes --]
On 11/21/20 8:50 PM, Joe Perches wrote:
>> What about moving the default to the end if the case, which is more common anyways:
>>
>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> []
>> @@ -295,16 +295,16 @@ static void peak_usb_write_bulk_callback(struct urb *urb)
>> netif_trans_update(netdev);
>> break;
>>
>>
>> - default:
>> - if (net_ratelimit())
>> - netdev_err(netdev, "Tx urb aborted (%d)\n",
>> - urb->status);
>> case -EPROTO:
>> case -ENOENT:
>> case -ECONNRESET:
>> case -ESHUTDOWN:
>> -
>> break;
>> +
>> + default:
>> + if (net_ratelimit())
>> + netdev_err(netdev, "Tx urb aborted (%d)\n",
>> + urb->status);
>
> That's fine and is more generally used style but this
> default: case should IMO also end with a break;
>
> + break;
I don't mind.
process/coding-style.rst is not totally clear about the break after the default,
if this is the lase one the switch statement.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net] devlink: Fix reload stats structure
From: Jakub Kicinski @ 2020-11-21 22:53 UTC (permalink / raw)
To: Moshe Shemesh; +Cc: David S. Miller, Jiri Pirko, netdev, linux-kernel
In-Reply-To: <1605879637-6114-1-git-send-email-moshe@mellanox.com>
On Fri, 20 Nov 2020 15:40:37 +0200 Moshe Shemesh wrote:
> Fix reload stats structure exposed to the user. Change stats structure
> hierarchy to have the reload action as a parent of the stat entry and
> then stat entry includes value per limit. This will also help to avoid
> string concatenation on iproute2 output.
>
> Reload stats structure before this fix:
> "stats": {
> "reload": {
> "driver_reinit": 2,
> "fw_activate": 1,
> "fw_activate_no_reset": 0
> }
> }
>
> After this fix:
> "stats": {
> "reload": {
> "driver_reinit": {
> "unspecified": 2
> },
> "fw_activate": {
> "unspecified": 1,
> "no_reset": 0
> }
> }
>
> Fixes: a254c264267e ("devlink: Add reload stats")
> Signed-off-by: Moshe Shemesh <moshe@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
At least try to fold the core networking code at 80 characters *please*.
You folded the comments at 86 chars, neither 100 nor 80.
^ permalink raw reply
* Re: [PATCH net] net/af_iucv: set correct sk_protocol for child sockets
From: patchwork-bot+netdevbpf @ 2020-11-21 22:50 UTC (permalink / raw)
To: Julian Wiedmann; +Cc: davem, kuba, netdev, linux-s390, hca, kgraul
In-Reply-To: <20201120100657.34407-1-jwi@linux.ibm.com>
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Fri, 20 Nov 2020 11:06:57 +0100 you wrote:
> Child sockets erroneously inherit their parent's sk_type (ie. SOCK_*),
> instead of the PF_IUCV protocol that the parent was created with in
> iucv_sock_create().
>
> We're currently not using sk->sk_protocol ourselves, so this shouldn't
> have much impact (except eg. getting the output in skb_dump() right).
>
> [...]
Here is the summary with links:
- [net] net/af_iucv: set correct sk_protocol for child sockets
https://git.kernel.org/netdev/net/c/c5dab0941fcd
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] net: bridge: switch to net core statistics counters handling
From: Jakub Kicinski @ 2020-11-21 22:41 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Roopa Prabhu, Nikolay Aleksandrov, David Miller, bridge,
netdev@vger.kernel.org
In-Reply-To: <9bad2be2-fd84-7c6e-912f-cee433787018@gmail.com>
On Fri, 20 Nov 2020 12:22:23 +0100 Heiner Kallweit wrote:
> Use netdev->tstats instead of a member of net_bridge for storing
> a pointer to the per-cpu counters. This allows us to use core
> functionality for statistics handling.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH net-next 0/5] net: hns3: misc updates for -next
From: patchwork-bot+netdevbpf @ 2020-11-21 22:40 UTC (permalink / raw)
To: tanhuazhong
Cc: davem, netdev, linux-kernel, salil.mehta, yisen.zhuang, linuxarm,
kuba
In-Reply-To: <1605863783-36995-1-git-send-email-tanhuazhong@huawei.com>
Hello:
This series was applied to netdev/net-next.git (refs/heads/master):
On Fri, 20 Nov 2020 17:16:18 +0800 you wrote:
> This series includes some misc updates for the HNS3 ethernet driver.
>
> #1 adds support for 1280 queues
> #2 adds mapping for BAR45 which is needed by RoCE client.
> #3 extend the interrupt resources.
> #4 add support to query firmware's calculated shaping parameters.
>
> [...]
Here is the summary with links:
- [net-next,1/5] net: hns3: add support for 1280 queues
https://git.kernel.org/netdev/net-next/c/9a5ef4aa5457
- [net-next,2/5] net: hns3: add support for mapping device memory
https://git.kernel.org/netdev/net-next/c/30ae7f8a6aa7
- [net-next,3/5] net: hns3: add support for pf querying new interrupt resources
https://git.kernel.org/netdev/net-next/c/3a6863e4e8ee
- [net-next,4/5] net: hns3: add support to utilize the firmware calculated shaping parameters
https://git.kernel.org/netdev/net-next/c/e364ad303fe3
- [net-next,5/5] net: hns3: adds debugfs to dump more info of shaping parameters
https://git.kernel.org/netdev/net-next/c/c331ecf1afc1
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] MAINTAINERS: Update page pool entry
From: Jakub Kicinski @ 2020-11-21 22:37 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: netdev, Ilias Apalodimas
In-Reply-To: <160586497895.2808766.2902017028647296556.stgit@firesoul>
On Fri, 20 Nov 2020 10:36:19 +0100 Jesper Dangaard Brouer wrote:
> Add some file F: matches that is related to page_pool.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> MAINTAINERS | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f827f504251b..efcdc68a03b1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13179,6 +13179,8 @@ L: netdev@vger.kernel.org
> S: Supported
> F: include/net/page_pool.h
> F: net/core/page_pool.c
> +F: include/trace/events/page_pool.h
> +F: Documentation/networking/page_pool.rst
Checkpatch says:
WARNING: Misordered MAINTAINERS entry - list file patterns in alphabetic order
#26: FILE: MAINTAINERS:13165:
F: net/core/page_pool.c
+F: include/trace/events/page_pool.h
WARNING: Misordered MAINTAINERS entry - list file patterns in alphabetic order
#27: FILE: MAINTAINERS:13166:
+F: include/trace/events/page_pool.h
+F: Documentation/networking/page_pool.rst
^ permalink raw reply
* Re: [Patch stable] netfilter: clear skb->next in NF_HOOK_LIST()
From: Florian Westphal @ 2020-11-21 22:22 UTC (permalink / raw)
To: Cong Wang
Cc: netdev, Cong Wang, liuzx, Florian Westphal, Edward Cree, stable,
Greg Kroah-Hartman
In-Reply-To: <20201121034317.577081-1-xiyou.wangcong@gmail.com>
Cong Wang <xiyou.wangcong@gmail.com> wrote:
> From: Cong Wang <cong.wang@bytedance.com>
>
> NF_HOOK_LIST() uses list_del() to remove skb from the linked list,
> however, it is not sufficient as skb->next still points to other
> skb. We should just call skb_list_del_init() to clear skb->next,
> like the rest places which using skb list.
>
> This has been fixed in upstream by commit ca58fbe06c54
> ("netfilter: add and use nf_hook_slow_list()").
Thanks Cong, agree with this change, afaics its applicable to 4.19.y and 5.4.y.
^ permalink raw reply
* Re: [PATCH] usbnet: ipheth: fix connectivity with iOS 14
From: Jakub Kicinski @ 2020-11-21 22:03 UTC (permalink / raw)
To: Yves-Alexis Perez
Cc: David S. Miller, Martin Habets, Luc Van Oostenryck,
Shannon Nelson, Michael S. Tsirkin, linux-usb, netdev,
linux-kernel, Matti Vuorela, stable
In-Reply-To: <20201119172439.94988-1-corsac@corsac.net>
On Thu, 19 Nov 2020 18:24:39 +0100 Yves-Alexis Perez wrote:
> Starting with iOS 14 released in September 2020, connectivity using the
> personal hotspot USB tethering function of iOS devices is broken.
>
> Communication between the host and the device (for example ICMP traffic
> or DNS resolution using the DNS service running in the device itself)
> works fine, but communication to endpoints further away doesn't work.
>
> Investigation on the matter shows that UDP and ICMP traffic from the
> tethered host is reaching the Internet at all. For TCP traffic there are
> exchanges between tethered host and server but packets are modified in
> transit leading to impossible communication.
>
> After some trials Matti Vuorela discovered that reducing the URB buffer
> size by two bytes restored the previous behavior. While a better
> solution might exist to fix the issue, since the protocol is not
> publicly documented and considering the small size of the fix, let's do
> that.
>
> Tested-by: Matti Vuorela <matti.vuorela@bitfactor.fi>
> Signed-off-by: Yves-Alexis Perez <corsac@corsac.net>
> Link: https://lore.kernel.org/linux-usb/CAAn0qaXmysJ9vx3ZEMkViv_B19ju-_ExN8Yn_uSefxpjS6g4Lw@mail.gmail.com/
> Link: https://github.com/libimobiledevice/libimobiledevice/issues/1038
Applied to net with the typo fixed, thanks!
^ permalink raw reply
* [PATCH net-next v2] compat: always include linux/compat.h from net/compat.h
From: Jakub Kicinski @ 2020-11-21 21:48 UTC (permalink / raw)
To: davem; +Cc: netdev, hch, arnd, Jakub Kicinski
We're about to do reshuffling in networking headers and
eliminate some implicit includes. This results in:
In file included from ../net/ipv4/netfilter/arp_tables.c:26:
include/net/compat.h:60:40: error: unknown type name ‘compat_uptr_t’; did you mean ‘compat_ptr_ioctl’?
struct sockaddr __user **save_addr, compat_uptr_t *ptr,
^~~~~~~~~~~~~
compat_ptr_ioctl
include/net/compat.h:61:4: error: unknown type name ‘compat_size_t’; did you mean ‘compat_sigset_t’?
compat_size_t *len);
^~~~~~~~~~~~~
compat_sigset_t
Currently net/compat.h depends on linux/compat.h being included
first. After the upcoming changes this would break the 32bit build.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
v2:
- correct the commit message
- remove the ifdef completely (Arnd)
---
include/net/compat.h | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/include/net/compat.h b/include/net/compat.h
index 745db0d605b6..84805bdc4435 100644
--- a/include/net/compat.h
+++ b/include/net/compat.h
@@ -5,8 +5,6 @@
struct sock;
-#if defined(CONFIG_COMPAT)
-
#include <linux/compat.h>
struct compat_msghdr {
@@ -48,14 +46,6 @@ struct compat_rtentry {
unsigned short rt_irtt; /* Initial RTT */
};
-#else /* defined(CONFIG_COMPAT) */
-/*
- * To avoid compiler warnings:
- */
-#define compat_msghdr msghdr
-#define compat_mmsghdr mmsghdr
-#endif /* defined(CONFIG_COMPAT) */
-
int __get_compat_msghdr(struct msghdr *kmsg, struct compat_msghdr __user *umsg,
struct sockaddr __user **save_addr, compat_uptr_t *ptr,
compat_size_t *len);
--
2.24.1
^ permalink raw reply related
* Re: [PATCH net-next] compat: always include linux/compat.h from net/compat.h
From: Jakub Kicinski @ 2020-11-21 21:48 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: David Miller, Networking, Christoph Hellwig, Arnd Bergmann
In-Reply-To: <CAK8P3a1sZ7CUwQ-fUCS-z4CkhgSiNqzPcc_MTc2D54-8vfmV=g@mail.gmail.com>
On Sat, 21 Nov 2020 22:25:35 +0100 Arnd Bergmann wrote:
> On Sat, Nov 21, 2020 at 6:52 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > We're about to do some reshuffling in networking headers and make
> > some of the file lose the implicit includes. This results in:
> >
> > In file included from net/ipv4/netfilter/arp_tables.c:26:
> > include/net/compat.h:57:23: error: conflicting types for ‘uintptr_t’
> > #define compat_uptr_t uintptr_t
> > ^~~~~~~~~
> > include/asm-generic/compat.h:22:13: note: in expansion of macro ‘compat_uptr_t’
> > typedef u32 compat_uptr_t;
> > ^~~~~~~~~~~~~
> > In file included from include/linux/limits.h:6,
> > from include/linux/kernel.h:7,
> > from net/ipv4/netfilter/arp_tables.c:14:
> > include/linux/types.h:37:24: note: previous declaration of ‘uintptr_t’ was here
> > typedef unsigned long uintptr_t;
> > ^~~~~~~~~
> >
> > Currently net/compat.h depends on linux/compat.h being included
> > first. After the upcoming changes this would break the 32bit build.
> >
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> > Not sure who officially maintains this. Arnd, Christoph any objections?
>
> Looks good to me. I would actually go one step further and completely
> remove this #ifdef, if possible. In the old days, it was not possible to
> include linux/compat.h on 32-bit architectures, but now this should just
> work without an #ifdef.
Indeed, that appears to work, v2 coming up, thanks!
^ permalink raw reply
* Re: [PATCH v2 net] ptp: clockmatrix: bug fix for idtcm_strverscmp
From: Jakub Kicinski @ 2020-11-21 21:40 UTC (permalink / raw)
To: min.li.xe; +Cc: richardcochran, netdev, linux-kernel
In-Reply-To: <1605757824-3292-1-git-send-email-min.li.xe@renesas.com>
On Wed, 18 Nov 2020 22:50:24 -0500 min.li.xe@renesas.com wrote:
> From: Min Li <min.li.xe@renesas.com>
>
> Feed kstrtou8 with NULL terminated string.
>
> Changes since v1:
> -Only strcpy 15 characters to leave 1 space for '\0'
>
> Signed-off-by: Min Li <min.li.xe@renesas.com>
> -static int idtcm_strverscmp(const char *ver1, const char *ver2)
> +static int idtcm_strverscmp(const char *version1, const char *version2)
> {
> u8 num1;
> u8 num2;
> int result = 0;
> + char ver1[16];
> + char ver2[16];
> + char *cur1;
> + char *cur2;
> + char *next1;
> + char *next2;
> +
> + strncpy(ver1, version1, 15);
> + strncpy(ver2, version2, 15);
> + cur1 = ver1;
> + cur2 = ver2;
Now there is no guarantee that the buffer is null terminated.
You need to use strscpy(ver... 16) or add ver[15] = '\0';
> /* loop through each level of the version string */
> while (result == 0) {
> + next1 = strchr(cur1, '.');
> + next2 = strchr(cur2, '.');
> +
> + /* kstrtou8 could fail for dot */
> + if (next1) {
> + *next1 = '\0';
> + next1++;
> + }
> +
> + if (next2) {
> + *next2 = '\0';
> + next2++;
> + }
> +
> /* extract leading version numbers */
> - if (kstrtou8(ver1, 10, &num1) < 0)
> + if (kstrtou8(cur1, 10, &num1) < 0)
> return -1;
>
> - if (kstrtou8(ver2, 10, &num2) < 0)
> + if (kstrtou8(cur2, 10, &num2) < 0)
> return -1;
>
> /* if numbers differ, then set the result */
> - if (num1 < num2)
> + if (num1 < num2) {
> result = -1;
Why do you set the result to something instead of just returning that
value? The kstrtou8() checks above just return value directly...
If you use a return you can save yourself all the else branches.
> - else if (num1 > num2)
> + } else if (num1 > num2) {
> result = 1;
> - else {
> + } else {
> /* if numbers are the same, go to next level */
> - ver1 = strchr(ver1, '.');
> - ver2 = strchr(ver2, '.');
> - if (!ver1 && !ver2)
> + if (!next1 && !next2)
> break;
> - else if (!ver1)
> + else if (!next1) {
> result = -1;
> - else if (!ver2)
> + } else if (!next2) {
> result = 1;
> - else {
> - ver1++;
> - ver2++;
> + } else {
> + cur1 = next1;
> + cur2 = next2;
> }
> }
> }
> +
> return result;
> }
>
^ permalink raw reply
* Re: [PATCH net-next] compat: always include linux/compat.h from net/compat.h
From: Arnd Bergmann @ 2020-11-21 21:25 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: David Miller, Networking, Christoph Hellwig, Arnd Bergmann
In-Reply-To: <20201121175224.1465831-1-kuba@kernel.org>
On Sat, Nov 21, 2020 at 6:52 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> We're about to do some reshuffling in networking headers and make
> some of the file lose the implicit includes. This results in:
>
> In file included from net/ipv4/netfilter/arp_tables.c:26:
> include/net/compat.h:57:23: error: conflicting types for ‘uintptr_t’
> #define compat_uptr_t uintptr_t
> ^~~~~~~~~
> include/asm-generic/compat.h:22:13: note: in expansion of macro ‘compat_uptr_t’
> typedef u32 compat_uptr_t;
> ^~~~~~~~~~~~~
> In file included from include/linux/limits.h:6,
> from include/linux/kernel.h:7,
> from net/ipv4/netfilter/arp_tables.c:14:
> include/linux/types.h:37:24: note: previous declaration of ‘uintptr_t’ was here
> typedef unsigned long uintptr_t;
> ^~~~~~~~~
>
> Currently net/compat.h depends on linux/compat.h being included
> first. After the upcoming changes this would break the 32bit build.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> Not sure who officially maintains this. Arnd, Christoph any objections?
Looks good to me. I would actually go one step further and completely
remove this #ifdef, if possible. In the old days, it was not possible to
include linux/compat.h on 32-bit architectures, but now this should just
work without an #ifdef.
Arnd
^ permalink raw reply
* Re: [PATCH v4] aquantia: Remove the build_skb path
From: Jakub Kicinski @ 2020-11-21 21:23 UTC (permalink / raw)
To: Ramsay, Lincoln
Cc: Florian Westphal, Igor Russkikh, David S. Miller,
netdev@vger.kernel.org, Dmitry Bogdanov
In-Reply-To: <20201121132204.43f9c4fb@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Sat, 21 Nov 2020 13:22:04 -0800 Jakub Kicinski wrote:
> On Thu, 19 Nov 2020 23:52:55 +0000 Ramsay, Lincoln wrote:
> > When performing IPv6 forwarding, there is an expectation that SKBs
> > will have some headroom. When forwarding a packet from the aquantia
> > driver, this does not always happen, triggering a kernel warning.
> >
> > aq_ring.c has this code (edited slightly for brevity):
> >
> > if (buff->is_eop && buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) {
> > skb = build_skb(aq_buf_vaddr(&buff->rxdata), AQ_CFG_RX_FRAME_MAX);
> > } else {
> > skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
> >
> > There is a significant difference between the SKB produced by these
> > 2 code paths. When napi_alloc_skb creates an SKB, there is a certain
> > amount of headroom reserved. However, this is not done in the
> > build_skb codepath.
> >
> > As the hardware buffer that build_skb is built around does not
> > handle the presence of the SKB header, this code path is being
> > removed and the napi_alloc_skb path will always be used. This code
> > path does have to copy the packet header into the SKB, but it adds
> > the packet data as a frag.
> >
> > Signed-off-by: Lincoln Ramsay <lincoln.ramsay@opengear.com>
>
> I was going to apply as a fix to net and stable but too many small nits
> here to pass. First of all please add a From: line at the beginning of
> the mail which matches the signoff (or use git-send-email, it'll get it
> right).
Ah, one more thing, this is the correct fixes tag, right?
Fixes: 018423e90bee ("net: ethernet: aquantia: Add ring support code")
Please add it right before the signoff line.
^ permalink raw reply
* Re: [PATCH v4] aquantia: Remove the build_skb path
From: Jakub Kicinski @ 2020-11-21 21:22 UTC (permalink / raw)
To: Ramsay, Lincoln
Cc: Florian Westphal, Igor Russkikh, David S. Miller,
netdev@vger.kernel.org, Dmitry Bogdanov
In-Reply-To: <CY4PR1001MB2311844FE8390F00A3363DEEE8E00@CY4PR1001MB2311.namprd10.prod.outlook.com>
On Thu, 19 Nov 2020 23:52:55 +0000 Ramsay, Lincoln wrote:
> When performing IPv6 forwarding, there is an expectation that SKBs
> will have some headroom. When forwarding a packet from the aquantia
> driver, this does not always happen, triggering a kernel warning.
>
> aq_ring.c has this code (edited slightly for brevity):
>
> if (buff->is_eop && buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) {
> skb = build_skb(aq_buf_vaddr(&buff->rxdata), AQ_CFG_RX_FRAME_MAX);
> } else {
> skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
>
> There is a significant difference between the SKB produced by these
> 2 code paths. When napi_alloc_skb creates an SKB, there is a certain
> amount of headroom reserved. However, this is not done in the
> build_skb codepath.
>
> As the hardware buffer that build_skb is built around does not
> handle the presence of the SKB header, this code path is being
> removed and the napi_alloc_skb path will always be used. This code
> path does have to copy the packet header into the SKB, but it adds
> the packet data as a frag.
>
> Signed-off-by: Lincoln Ramsay <lincoln.ramsay@opengear.com>
I was going to apply as a fix to net and stable but too many small nits
here to pass. First of all please add a From: line at the beginning of
the mail which matches the signoff (or use git-send-email, it'll get it
right).
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index 4f913658eea4..425e8e5afec7 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -413,85 +413,64 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
> buff->rxdata.pg_off,
> buff->len, DMA_FROM_DEVICE);
>
> - /* for single fragment packets use build_skb() */
> - if (buff->is_eop &&
> - buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) {
> - skb = build_skb(aq_buf_vaddr(&buff->rxdata),
> + skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
> + if (unlikely(!skb)) {
> + u64_stats_update_begin(&self->stats.rx.syncp);
> + self->stats.rx.skb_alloc_fails++;
> + u64_stats_update_end(&self->stats.rx.syncp);
> + err = -ENOMEM;
> + goto err_exit;
> + }
> + if (is_ptp_ring)
> + buff->len -=
> + aq_ptp_extract_ts(self->aq_nic, skb,
> + aq_buf_vaddr(&buff->rxdata),
> + buff->len);
Align continuations of the lines under '(' like:
./scripts/checkpatch.pl --max-line-length=80 --strict
is asking you to.
> + hdr_len = buff->len;
> + if (hdr_len > AQ_CFG_RX_HDR_SIZE)
> + hdr_len = eth_get_headlen(skb->dev,
> + aq_buf_vaddr(&buff->rxdata),
> + AQ_CFG_RX_HDR_SIZE);
ditto
> + memcpy(__skb_put(skb, hdr_len), aq_buf_vaddr(&buff->rxdata),
> + ALIGN(hdr_len, sizeof(long)));
And here, etc.
> + if (buff->len - hdr_len > 0) {
> + skb_add_rx_frag(skb, 0, buff->rxdata.page,
> + buff->rxdata.pg_off + hdr_len,
> + buff->len - hdr_len,
> + if (!buff->is_eop) {
> + buff_ = buff;
> + i = 1U;
> + do {
> + next_ = buff_->next,
The end of this line should be a semicolon.
> + buff_ = &self->buff_ring[next_];
Thanks!
^ permalink raw reply
* Re: [PATCH] cxgb4: Fix build failure when CONFIG_TLS=m
From: Jakub Kicinski @ 2020-11-21 21:12 UTC (permalink / raw)
To: Tom Seewald; +Cc: netdev, linux-kernel, davem, ayush.sawal, rajur
In-Reply-To: <20201120192528.615-1-tseewald@gmail.com>
On Fri, 20 Nov 2020 13:25:28 -0600 Tom Seewald wrote:
> After commit 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> whenever CONFIG_TLS=m and CONFIG_CHELSIO_T4=y, the following build
> failure occurs:
>
> ld: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.o: in function
> `cxgb_select_queue':
> cxgb4_main.c:(.text+0x2dac): undefined reference to `tls_validate_xmit_skb'
>
> Fix this by ensuring that if TLS is set to be a module, CHELSIO_T4 will
> also be compiled as a module. As otherwise the cxgb4 driver will not be
> able to access TLS' symbols.
>
> Fixes: 9d2e5e9eeb59 ("cxgb4/ch_ktls: decrypted bit is not enough")
> Signed-off-by: Tom Seewald <tseewald@gmail.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCHv3] bonding: wait for sysfs kobject destruction before freeing struct slave
From: Jakub Kicinski @ 2020-11-21 21:09 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jamie Iles, netdev, Qiushi Wu, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek
In-Reply-To: <X7fjR8ZB6BVwKS++@kroah.com>
On Fri, 20 Nov 2020 16:39:51 +0100 Greg Kroah-Hartman wrote:
> On Fri, Nov 20, 2020 at 02:28:27PM +0000, Jamie Iles wrote:
> > syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
> > struct slave device could result in the following splat:
> > This is a potential use-after-free if the sysfs nodes are being accessed
> > whilst removing the struct slave, so wait for the object destruction to
> > complete before freeing the struct slave itself.
>
> Nice, it looks like it should have always been done this way!
>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Applied to net, thanks!
^ permalink raw reply
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Jakub Kicinski @ 2020-11-21 21:02 UTC (permalink / raw)
To: Johannes Berg
Cc: Florian Westphal, Ido Schimmel, Aleksandr Nogikh, davem, edumazet,
andreyknvl, dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <86c6369a937c760e374c78f5252ffc67cf67b1e1.camel@sipsolutions.net>
On Sat, 21 Nov 2020 21:58:37 +0100 Johannes Berg wrote:
> On Sat, 2020-11-21 at 12:55 -0800, Jakub Kicinski wrote:
> > It is more complicated. We can go back to an skb field if this work is
> > expected to yield results for mac80211. Would you mind sending a patch?
>
> I can do that, but I'm not going to be able to do it now/tonight (GMT+1
> here), so probably only Monday/Tuesday or so, sorry.
Oh yea, no worries, took someone a month to notice this is broken,
as long as it's fixed before the merge window it's fine ;)
^ permalink raw reply
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Johannes Berg @ 2020-11-21 20:58 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Florian Westphal, Ido Schimmel, Aleksandr Nogikh, davem, edumazet,
andreyknvl, dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <20201121125508.4d526dd0@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Sat, 2020-11-21 at 12:55 -0800, Jakub Kicinski wrote:
> [snip]
> Ack, you have to figure out all the places anyway, the question is
> whether you put probes there or calls in the source code.
>
> Shifting the maintenance burden but also BPF is flexibility.
Yeah, true. Though I'd argue also visibility - this stuff is pretty
simple now, if it gets into lots of lines of BPF code to track it that
is maintained "elsewhere", we won't see the bugs in it :-)
And it's kinda a thing that we as kernel developers _should_ be the ones
looking at since it's testing our code.
> Yup, the point is you can feed a raw skb pointer (and all other
> possible context you may want) to a BPF prog in kcov_remote_start()
> and let BPF/BTF give you the handle it recorded in its maps.
Yeah, it's possible. Personally, I don't think it's worth the
complexity.
> It is more complicated. We can go back to an skb field if this work is
> expected to yield results for mac80211. Would you mind sending a patch?
I can do that, but I'm not going to be able to do it now/tonight (GMT+1
here), so probably only Monday/Tuesday or so, sorry.
johannes
^ permalink raw reply
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Jakub Kicinski @ 2020-11-21 20:55 UTC (permalink / raw)
To: Johannes Berg
Cc: Florian Westphal, Ido Schimmel, Aleksandr Nogikh, davem, edumazet,
andreyknvl, dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <106fc65f0459bc316e89beaf6bd71e823c4c01b7.camel@sipsolutions.net>
On Sat, 21 Nov 2020 20:30:44 +0100 Johannes Berg wrote:
> On Sat, 2020-11-21 at 10:35 -0800, Jakub Kicinski wrote:
> > On Sat, 21 Nov 2020 19:12:21 +0100 Johannes Berg wrote:
> > > > So I'm leaning towards reverting the whole thing. You can attach
> > > > kretprobes and record the information you need in BPF maps.
> > >
> > > I'm not going to object to reverting it (and perhaps redoing it better
> > > later), but I will point out that kretprobe isn't going to work, you
> > > eventually need kcov_remote_start() to be called in strategic points
> > > before processing the skb after it bounced through the system.
> > >
> > > IOW, it's not really about serving userland, it's about enabling (and
> > > later disabling) coverage collection for the bits of code it cares
> > > about, mostly because collecting it for _everything_ is going to be too
> > > slow and will mess up the data since for coverage guided fuzzing you
> > > really need the reported coverage data to be only about the injected
> > > fuzz data...
> >
> > All you need is make kcov_remote_start_common() be BPF-able, like
> > the LSM hooks are now, right? And then BPF can return whatever handle
> > it pleases.
>
> Not sure I understand. Are you saying something should call
> "kcov_remote_start_common()" with, say, the SKB, and leave it to a mass
> of bpf hooks to figure out where the SKB got cloned or copied or
> whatnot, track that in a map, and then ... no, wait, I don't really see
> what you mean, sorry.
>
> IIUC, fundamentally, you have this:
>
> - at the beginning, a task is tagged with "please collect coverage
> data for this handle"
Write the tag into task local storage, or map indexed by PID.
> - this task creates an SKB, etc, and all of the code that this task
> executes is captured and the coverage data is reported
kprobe the right places to record the skb -> handle mapping.
> - However, the SKB traverses lots of things, gets copied, cloned, or
> whatnot, and eventually leaves the annotated task, say for further
> processing in softirq context or elsewhere.
Which is fine.
> Now since the whole point is to see what chaos this SKB created from
> beginning (allocation) to end (free), since it was filled with fuzzed
> data, you now have to figure out where to pick back up when the SKB is
> processed further.
>
> This is what the infrastructure was meant to solve. But note that the
> SKB might be further cloned etc, so in order to track it you'd have to
> (out-of-band) figure out all the possible places where it could
> be reallocated, any time the skb pointer could change.
Ack, you have to figure out all the places anyway, the question is
whether you put probes there or calls in the source code.
Shifting the maintenance burden but also BPF is flexibility.
> Then, when you know you've got interesting code on your hands, like in
> mac80211 that was annotated in patch 3 here, you basically say
>
> "oohhh, this SKB was annotated before, let's continue capturing
> coverage data here"
>
> (and turn it off again later by the corresponding kcov_remote_stop().
Yup, the point is you can feed a raw skb pointer (and all other
possible context you may want) to a BPF prog in kcov_remote_start()
and let BPF/BTF give you the handle it recorded in its maps.
> So the only way I could _possibly_ see how to do this would be to
>
> * capture all possible places where the skb pointer can change
> * still call something like skb_get_kcov_handle() but let it call out
> to a BPF program to query a map or something to figure out if this
> SKB has a handle attached to it
>
> > Or if you don't like BPF or what to KCOV BPF itself in the future you
> > can roll your own mechanism. The point is - this should be relatively
> > easily doable out of line...
>
> Seems pretty complicated to me though ...
It is more complicated. We can go back to an skb field if this work is
expected to yield results for mac80211. Would you mind sending a patch?
^ 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