* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: James Bottomley @ 2020-11-22 19:12 UTC (permalink / raw)
To: Joe Perches, Kees Cook, Jakub Kicinski
Cc: Gustavo A. R. Silva, linux-kernel, alsa-devel, amd-gfx, bridge,
ceph-devel, cluster-devel, coreteam, devel, dm-devel, drbd-dev,
dri-devel, GR-everest-linux-l2, GR-Linux-NIC-Dev, intel-gfx,
intel-wired-lan, keyrings, linux1394-devel, linux-acpi, linux-afs,
linux-arm-kernel, linux-arm-msm, linux-atm-general, linux-block,
linux-can, linux-cifs, linux-crypto, linux-decnet-user,
linux-ext4, linux-fbdev, linux-geode, linux-gpio, linux-hams,
linux-hwmon, linux-i3c, linux-ide, linux-iio, linux-input,
linux-integrity, linux-mediatek, linux-media, linux-mmc, linux-mm,
linux-mtd, linux-nfs, linux-rdma, linux-renesas-soc, linux-scsi,
linux-sctp, linux-security-module, linux-stm32, linux-usb,
linux-watchdog, linux-wireless, netdev, netfilter-devel, nouveau,
op-tee, oss-drivers, patches, rds-devel, reiserfs-devel,
samba-technical, selinux, target-devel, tipc-discussion,
usb-storage, virtualization, wcn36xx, x86, xen-devel,
linux-hardening, Nick Desaulniers, Nathan Chancellor,
Miguel Ojeda
In-Reply-To: <ca071decb87cc7e905411423c05a48f9fd2f58d7.camel@perches.com>
On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote:
> On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote:
> > Please tell me our reward for all this effort isn't a single
> > missing error print.
>
> There were quite literally dozens of logical defects found
> by the fallthrough additions. Very few were logging only.
So can you give us the best examples (or indeed all of them if someone
is keeping score)? hopefully this isn't a US election situation ...
James
^ permalink raw reply
* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: Joe Perches @ 2020-11-22 18:25 UTC (permalink / raw)
To: James Bottomley, Kees Cook, Jakub Kicinski
Cc: Gustavo A. R. Silva, linux-kernel, alsa-devel, amd-gfx, bridge,
ceph-devel, cluster-devel, coreteam, devel, dm-devel, drbd-dev,
dri-devel, GR-everest-linux-l2, GR-Linux-NIC-Dev, intel-gfx,
intel-wired-lan, keyrings, linux1394-devel, linux-acpi, linux-afs,
linux-arm-kernel, linux-arm-msm, linux-atm-general, linux-block,
linux-can, linux-cifs, linux-crypto, linux-decnet-user,
linux-ext4, linux-fbdev, linux-geode, linux-gpio, linux-hams,
linux-hwmon, linux-i3c, linux-ide, linux-iio, linux-input,
linux-integrity, linux-mediatek, linux-media, linux-mmc, linux-mm,
linux-mtd, linux-nfs, linux-rdma, linux-renesas-soc, linux-scsi,
linux-sctp, linux-security-module, linux-stm32, linux-usb,
linux-watchdog, linux-wireless, netdev, netfilter-devel, nouveau,
op-tee, oss-drivers, patches, rds-devel, reiserfs-devel,
samba-technical, selinux, target-devel, tipc-discussion,
usb-storage, virtualization, wcn36xx, x86, xen-devel,
linux-hardening, Nick Desaulniers, Nathan Chancellor,
Miguel Ojeda
In-Reply-To: <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com>
On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote:
> Please tell me
> our reward for all this effort isn't a single missing error print.
There were quite literally dozens of logical defects found
by the fallthrough additions. Very few were logging only.
^ permalink raw reply
* Re: [RFC] MAINTAINERS tag for cleanup robot
From: Joe Perches @ 2020-11-22 18:23 UTC (permalink / raw)
To: James Bottomley, Tom Rix, Matthew Wilcox
Cc: clang-built-linux, linux-hyperv, linux-kernel, xen-devel,
tboot-devel, kvm, linux-crypto, linux-acpi, devel, amd-gfx,
dri-devel, intel-gfx, netdev, linux-media, MPT-FusionLinux.pdl,
linux-scsi, linux-wireless, ibm-acpi-devel, platform-driver-x86,
linux-usb, linux-omap, linux-fbdev, ecryptfs, linux-fsdevel,
cluster-devel, linux-mtd, keyrings, netfilter-devel, coreteam,
alsa-devel, bpf, linux-bluetooth, linux-nfs, patches
In-Reply-To: <751803306cd957d0e7ef6a4fc3dbf12ebceaba92.camel@HansenPartnership.com>
On Sun, 2020-11-22 at 08:49 -0800, James Bottomley wrote:
> We can enforce sysfs_emit going forwards
> using tools like checkpatch
It's not really possible for checkpatch to find or warn about
sysfs uses of sprintf. checkpatch is really just a trivial
line-by-line parser and it has no concept of code intent.
It just can't warn on every use of the sprintf family.
There are just too many perfectly valid uses.
> but there's no benefit and a lot of harm to
> be done by trying to churn the entire tree
Single uses of sprintf for sysfs is not really any problem.
But likely there are still several possible overrun sprintf/snprintf
paths in sysfs. Some of them are very obscure and unlikely to be
found by a robot as the logic for sysfs buf uses can be fairly twisty.
But provably correct conversions IMO _should_ be done and IMO churn
considerations should generally have less importance.
^ permalink raw reply
* Re: [RFC] MAINTAINERS tag for cleanup robot
From: Joe Perches @ 2020-11-22 18:22 UTC (permalink / raw)
To: Tom Rix, clang-built-linux
Cc: linux-hyperv, linux-kernel, xen-devel, tboot-devel, kvm,
linux-crypto, linux-acpi, devel, amd-gfx, dri-devel, intel-gfx,
netdev, linux-media, MPT-FusionLinux.pdl, linux-scsi,
linux-wireless, ibm-acpi-devel, platform-driver-x86, linux-usb,
linux-omap, linux-fbdev, ecryptfs, linux-fsdevel, cluster-devel,
linux-mtd, keyrings, netfilter-devel, coreteam, alsa-devel, bpf,
linux-bluetooth, linux-nfs, patches
In-Reply-To: <6e8c1926-4209-8f10-d0f9-72c875a85a88@redhat.com>
On Sun, 2020-11-22 at 08:33 -0800, Tom Rix wrote:
> On 11/21/20 9:10 AM, Joe Perches wrote:
> > On Sat, 2020-11-21 at 08:50 -0800, trix@redhat.com wrote:
> > > A difficult part of automating commits is composing the subsystem
> > > preamble in the commit log. For the ongoing effort of a fixer producing
> > > one or two fixes a release the use of 'treewide:' does not seem appropriate.
> > >
> > > It would be better if the normal prefix was used. Unfortunately normal is
> > > not consistent across the tree.
> > >
> > > So I am looking for comments for adding a new tag to the MAINTAINERS file
> > >
> > > D: Commit subsystem prefix
> > >
> > > ex/ for FPGA DFL DRIVERS
> > >
> > > D: fpga: dfl:
> > I'm all for it. Good luck with the effort. It's not completely trivial.
> >
> > From a decade ago:
> >
> > https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/
> >
> > (and that thread started with extra semicolon patches too)
>
> Reading the history, how about this.
>
> get_maintainer.pl outputs a single prefix, if multiple files have the
> same prefix it works, if they don't its an error.
>
> Another script 'commit_one_file.sh' does the call to get_mainainter.pl
> to get the prefix and be called by run-clang-tools.py to get the fixer
> specific message.
It's not whether the script used is get_maintainer or any other script,
the question is really if the MAINTAINERS file is the appropriate place
to store per-subsystem patch specific prefixes.
It is.
Then the question should be how are the forms described and what is the
inheritance priority. My preference would be to have a default of
inherit the parent base and add basename(subsystem dirname).
Commit history seems to have standardized on using colons as the separator
between the commit prefix and the subject.
A good mechanism to explore how various subsystems have uses prefixes in
the past might be something like:
$ git log --no-merges --pretty='%s' -<commit_count> <subsystem_path> | \
perl -n -e 'print substr($_, 0, rindex($_, ":") + 1) . "\n";' | \
sort | uniq -c | sort -rn
Using 10000 for commit_count and drivers/scsi for subsystem_path, the
top 40 entries are below:
About 1% don't have a colon, and there is no real consistency even
within individual drivers below scsi. For instance, qla2xxx:
1 814 scsi: qla2xxx:
2 691 scsi: lpfc:
3 389 scsi: hisi_sas:
4 354 scsi: ufs:
5 339 scsi:
6 291 qla2xxx:
7 256 scsi: megaraid_sas:
8 249 scsi: mpt3sas:
9 200 hpsa:
10 190 scsi: aacraid:
11 174 lpfc:
12 153 scsi: qedf:
13 144 scsi: smartpqi:
14 139 scsi: cxlflash:
15 122 scsi: core:
16 110 [SCSI] qla2xxx:
17 108 ncr5380:
18 98 scsi: hpsa:
19 97
20 89 treewide:
21 88 mpt3sas:
22 86 scsi: libfc:
23 85 scsi: qedi:
24 84 scsi: be2iscsi:
25 81 [SCSI] qla4xxx:
26 81 hisi_sas:
27 81 block:
28 75 megaraid_sas:
29 71 scsi: sd:
30 69 [SCSI] hpsa:
31 68 cxlflash:
32 65 scsi: libsas:
33 65 scsi: fnic:
34 61 scsi: scsi_debug:
35 60 scsi: arcmsr:
36 57 be2iscsi:
37 53 atp870u:
38 51 scsi: bfa:
39 50 scsi: storvsc:
40 48 sd:
^ permalink raw reply
* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: James Bottomley @ 2020-11-22 18:21 UTC (permalink / raw)
To: Kees Cook, Jakub Kicinski
Cc: Gustavo A. R. Silva, linux-kernel, alsa-devel, amd-gfx, bridge,
ceph-devel, cluster-devel, coreteam, devel, dm-devel, drbd-dev,
dri-devel, GR-everest-linux-l2, GR-Linux-NIC-Dev, intel-gfx,
intel-wired-lan, keyrings, linux1394-devel, linux-acpi, linux-afs,
linux-arm-kernel, linux-arm-msm, linux-atm-general, linux-block,
linux-can, linux-cifs, linux-crypto, linux-decnet-user,
linux-ext4, linux-fbdev, linux-geode, linux-gpio, linux-hams,
linux-hwmon, linux-i3c, linux-ide, linux-iio, linux-input,
linux-integrity, linux-mediatek, linux-media, linux-mmc, linux-mm,
linux-mtd, linux-nfs, linux-rdma, linux-renesas-soc, linux-scsi,
linux-sctp, linux-security-module, linux-stm32, linux-usb,
linux-watchdog, linux-wireless, netdev, netfilter-devel, nouveau,
op-tee, oss-drivers, patches, rds-devel, reiserfs-devel,
samba-technical, selinux, target-devel, tipc-discussion,
usb-storage, virtualization, wcn36xx, x86, xen-devel,
linux-hardening, Nick Desaulniers, Nathan Chancellor,
Miguel Ojeda, Joe Perches
In-Reply-To: <202011220816.8B6591A@keescook>
On Sun, 2020-11-22 at 08:17 -0800, Kees Cook wrote:
> On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote:
> > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote:
> > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:
> > > > > This series aims to fix almost all remaining fall-through
> > > > > warnings in order to enable -Wimplicit-fallthrough for Clang.
> > > > >
> > > > > In preparation to enable -Wimplicit-fallthrough for Clang,
> > > > > explicitly add multiple break/goto/return/fallthrough
> > > > > statements instead of just letting the code fall through to
> > > > > the next case.
> > > > >
> > > > > Notice that in order to enable -Wimplicit-fallthrough for
> > > > > Clang, this change[1] is meant to be reverted at some point.
> > > > > So, this patch helps to move in that direction.
> > > > >
> > > > > Something important to mention is that there is currently a
> > > > > discrepancy between GCC and Clang when dealing with switch
> > > > > fall-through to empty case statements or to cases that only
> > > > > contain a break/continue/return statement[2][3][4].
> > > >
> > > > Are we sure we want to make this change? Was it discussed
> > > > before?
> > > >
> > > > Are there any bugs Clangs puritanical definition of fallthrough
> > > > helped find?
> > > >
> > > > IMVHO compiler warnings are supposed to warn about issues that
> > > > could be bugs. Falling through to default: break; can hardly be
> > > > a bug?!
> > >
> > > It's certainly a place where the intent is not always clear. I
> > > think this makes all the cases unambiguous, and doesn't impact
> > > the machine code, since the compiler will happily optimize away
> > > any behavioral redundancy.
> >
> > If none of the 140 patches here fix a real bug, and there is no
> > change to machine code then it sounds to me like a W=2 kind of a
> > warning.
>
> FWIW, this series has found at least one bug so far:
> https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=Kr_CQ@mail.gmail.com/
Well, it's a problem in an error leg, sure, but it's not a really
compelling reason for a 141 patch series, is it? All that fixing this
error will do is get the driver to print "oh dear there's a problem"
under four more conditions than it previously did.
We've been at this for three years now with nearly a thousand patches,
firstly marking all the fall throughs with /* fall through */ and later
changing it to fallthrough. At some point we do have to ask if the
effort is commensurate with the protection afforded. Please tell me
our reward for all this effort isn't a single missing error print.
James
^ permalink raw reply
* [PATCH] sctp: Fix some typo
From: Christophe JAILLET @ 2020-11-22 18:07 UTC (permalink / raw)
To: vyasevich, nhorman, marcelo.leitner, davem, kuba
Cc: linux-sctp, netdev, linux-kernel, kernel-janitors,
Christophe JAILLET
s/tranport/transport/
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
net/sctp/transport.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index 60fcf31cdcfb..bf0ac467e757 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -8,7 +8,7 @@
*
* This file is part of the SCTP kernel implementation
*
- * This module provides the abstraction for an SCTP tranport representing
+ * This module provides the abstraction for an SCTP transport representing
* a remote transport address. For local transport addresses, we just use
* union sctp_addr.
*
@@ -123,7 +123,7 @@ void sctp_transport_free(struct sctp_transport *transport)
/* Delete the T3_rtx timer if it's active.
* There is no point in not doing this now and letting
* structure hang around in memory since we know
- * the tranport is going away.
+ * the transport is going away.
*/
if (del_timer(&transport->T3_rtx_timer))
sctp_transport_put(transport);
--
2.27.0
^ permalink raw reply related
* [PATCH] ath11k: Fix an error handling path
From: Christophe JAILLET @ 2020-11-22 17:39 UTC (permalink / raw)
To: kvalo, davem, kuba, gseset, mkenna, slakkavalli, gsamin, pradeepc
Cc: ath11k, linux-wireless, netdev, linux-kernel, kernel-janitors,
Christophe JAILLET
If 'kzalloc' fails, we must return an error code.
While at it, remove a useless initialization of 'err' which could hide the
issue.
Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
drivers/net/wireless/ath/ath11k/qmi.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
index c2b165158225..99a88ca83dea 100644
--- a/drivers/net/wireless/ath/ath11k/qmi.c
+++ b/drivers/net/wireless/ath/ath11k/qmi.c
@@ -1585,15 +1585,17 @@ static int ath11k_qmi_fw_ind_register_send(struct ath11k_base *ab)
struct qmi_wlanfw_ind_register_resp_msg_v01 *resp;
struct qmi_handle *handle = &ab->qmi.handle;
struct qmi_txn txn;
- int ret = 0;
+ int ret;
req = kzalloc(sizeof(*req), GFP_KERNEL);
if (!req)
return -ENOMEM;
resp = kzalloc(sizeof(*resp), GFP_KERNEL);
- if (!resp)
+ if (!resp) {
+ ret = -ENOMEM;
goto resp_out;
+ }
req->client_id_valid = 1;
req->client_id = QMI_WLANFW_CLIENT_ID;
--
2.27.0
^ permalink raw reply related
* [PATCH iproute2-next] ip: add IP_LIB_DIR environment variable
From: Sergey Ryazanov @ 2020-11-22 17:39 UTC (permalink / raw)
To: David Ahern; +Cc: netdev
Do not hardcode /usr/lib/ip as a path and allow libraries path
configuration in run-time.
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
ip/ip.c | 15 +++++++++++++++
ip/ip_common.h | 2 ++
ip/iplink.c | 5 +----
3 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/ip/ip.c b/ip/ip.c
index 5e31957f..38600e51 100644
--- a/ip/ip.c
+++ b/ip/ip.c
@@ -25,6 +25,10 @@
#include "color.h"
#include "rt_names.h"
+#ifndef LIBDIR
+#define LIBDIR "/usr/lib"
+#endif
+
int preferred_family = AF_UNSPEC;
int human_readable;
int use_iec;
@@ -41,6 +45,17 @@ bool do_all;
struct rtnl_handle rth = { .fd = -1 };
+const char *get_ip_lib_dir(void)
+{
+ const char *lib_dir;
+
+ lib_dir = getenv("IP_LIB_DIR");
+ if (!lib_dir)
+ lib_dir = LIBDIR "/ip";
+
+ return lib_dir;
+}
+
static void usage(void) __attribute__((noreturn));
static void usage(void)
diff --git a/ip/ip_common.h b/ip/ip_common.h
index d604f755..227eddd3 100644
--- a/ip/ip_common.h
+++ b/ip/ip_common.h
@@ -27,6 +27,8 @@ struct link_filter {
int target_nsid;
};
+const char *get_ip_lib_dir(void);
+
int get_operstate(const char *name);
int print_linkinfo(struct nlmsghdr *n, void *arg);
int print_addrinfo(struct nlmsghdr *n, void *arg);
diff --git a/ip/iplink.c b/ip/iplink.c
index d6b766de..4250b2c3 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -34,9 +34,6 @@
#include "namespace.h"
#define IPLINK_IOCTL_COMPAT 1
-#ifndef LIBDIR
-#define LIBDIR "/usr/lib"
-#endif
#ifndef GSO_MAX_SIZE
#define GSO_MAX_SIZE 65536
@@ -157,7 +154,7 @@ struct link_util *get_link_kind(const char *id)
if (strcmp(l->id, id) == 0)
return l;
- snprintf(buf, sizeof(buf), LIBDIR "/ip/link_%s.so", id);
+ snprintf(buf, sizeof(buf), "%s/link_%s.so", get_ip_lib_dir(), id);
dlh = dlopen(buf, RTLD_LAZY);
if (dlh == NULL) {
/* look in current binary, only open once */
--
2.26.2
^ permalink raw reply related
* Re: [Patch stable] netfilter: clear skb->next in NF_HOOK_LIST()
From: Sasha Levin @ 2020-11-22 17:24 UTC (permalink / raw)
To: Florian Westphal
Cc: Cong Wang, netdev, Cong Wang, liuzx, Edward Cree, stable,
Greg Kroah-Hartman
In-Reply-To: <20201121222249.GU15137@breakpoint.cc>
On Sat, Nov 21, 2020 at 11:22:49PM +0100, Florian Westphal wrote:
>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.
Queued for 5.4 and 4.19, thanks!
--
Thanks,
Sasha
^ permalink raw reply
* [PATCH 2/2] ath10k: Release some resources in an error handling path
From: Christophe JAILLET @ 2020-11-22 17:03 UTC (permalink / raw)
To: kvalo, davem, kuba, erik.stromdahl
Cc: ath10k, linux-wireless, netdev, linux-kernel, kernel-janitors,
Christophe JAILLET
Should an error occur after calling 'ath10k_usb_create()', it should be
undone by a corresponding 'ath10k_usb_destroy()' call
Fixes: 4db66499df91 ("ath10k: add initial USB support")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is speculative and compile tested only.
---
drivers/net/wireless/ath/ath10k/usb.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/usb.c b/drivers/net/wireless/ath/ath10k/usb.c
index 0b47c3a09794..19b9c27e30e2 100644
--- a/drivers/net/wireless/ath/ath10k/usb.c
+++ b/drivers/net/wireless/ath/ath10k/usb.c
@@ -1011,7 +1011,7 @@ static int ath10k_usb_probe(struct usb_interface *interface,
ret = ath10k_core_register(ar, &bus_params);
if (ret) {
ath10k_warn(ar, "failed to register driver core: %d\n", ret);
- goto err;
+ goto err_usb_destroy;
}
/* TODO: remove this once USB support is fully implemented */
@@ -1019,6 +1019,9 @@ static int ath10k_usb_probe(struct usb_interface *interface,
return 0;
+err_usb_destroy:
+ ath10k_usb_destroy(ar);
+
err:
ath10k_core_destroy(ar);
--
2.27.0
^ permalink raw reply related
* [PATCH 1/2] ath10k: Fix an error handling path
From: Christophe JAILLET @ 2020-11-22 17:03 UTC (permalink / raw)
To: kvalo, davem, kuba, erik.stromdahl
Cc: ath10k, linux-wireless, netdev, linux-kernel, kernel-janitors,
Christophe JAILLET
If 'ath10k_usb_create()' fails, we should release some resources and report
an error instead of silently continuing.
Fixes: 4db66499df91 ("ath10k: add initial USB support")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
drivers/net/wireless/ath/ath10k/usb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/usb.c b/drivers/net/wireless/ath/ath10k/usb.c
index 05a620ff6fe2..0b47c3a09794 100644
--- a/drivers/net/wireless/ath/ath10k/usb.c
+++ b/drivers/net/wireless/ath/ath10k/usb.c
@@ -997,6 +997,8 @@ static int ath10k_usb_probe(struct usb_interface *interface,
ar_usb = ath10k_usb_priv(ar);
ret = ath10k_usb_create(ar, interface);
+ if (ret)
+ goto err;
ar_usb->ar = ar;
ar->dev_id = product_id;
--
2.27.0
^ permalink raw reply related
* Re: [RFC] MAINTAINERS tag for cleanup robot
From: James Bottomley @ 2020-11-22 16:49 UTC (permalink / raw)
To: Tom Rix, Matthew Wilcox
Cc: joe, clang-built-linux, linux-hyperv, linux-kernel, xen-devel,
tboot-devel, kvm, linux-crypto, linux-acpi, devel, amd-gfx,
dri-devel, intel-gfx, netdev, linux-media, MPT-FusionLinux.pdl,
linux-scsi, linux-wireless, ibm-acpi-devel, platform-driver-x86,
linux-usb, linux-omap, linux-fbdev, ecryptfs, linux-fsdevel,
cluster-devel, linux-mtd, keyrings, netfilter-devel, coreteam,
alsa-devel, bpf, linux-bluetooth, linux-nfs, patches
In-Reply-To: <0819ce06-c462-d4df-d3d9-14931dc5aefc@redhat.com>
On Sun, 2020-11-22 at 08:10 -0800, Tom Rix wrote:
> On 11/22/20 6:56 AM, Matthew Wilcox wrote:
> > On Sun, Nov 22, 2020 at 06:46:46AM -0800, Tom Rix wrote:
> > > On 11/21/20 7:23 PM, Matthew Wilcox wrote:
> > > > On Sat, Nov 21, 2020 at 08:50:58AM -0800, trix@redhat.com
> > > > wrote:
> > > > > The fixer review is
> > > > > https://reviews.llvm.org/D91789
> > > > >
> > > > > A run over allyesconfig for x86_64 finds 62 issues, 5 are
> > > > > false positives. The false positives are caused by macros
> > > > > passed to other macros and by some macro expansions that did
> > > > > not have an extra semicolon.
> > > > >
> > > > > This cleans up about 1,000 of the current 10,000 -Wextra-
> > > > > semi-stmt warnings in linux-next.
> > > > Are any of them not false-positives? It's all very well to
> > > > enable stricter warnings, but if they don't fix any bugs,
> > > > they're just churn.
> > > >
> > > While enabling additional warnings may be a side effect of this
> > > effort
> > >
> > > the primary goal is to set up a cleaning robot. After that a
> > > refactoring robot.
> > Why do we need such a thing? Again, it sounds like more churn.
> > It's really annoying when I'm working on something important that
> > gets derailed by pointless churn. Churn also makes it harder to
> > backport patches to earlier kernels.
> >
> A refactoring example on moving to treewide, consistent use of a new
> api may help.
>
> Consider
>
> 2efc459d06f1630001e3984854848a5647086232
>
> sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output
>
> A new api for printing in the sysfs. How do we use it treewide ?
>
> Done manually, it would be a heroic effort requiring high level
> maintainers pushing and likely only get partially done.
>
> If a refactoring programatic fixit is done and validated on a one
> subsystem, it can run on all the subsystems.
>
> The effort is a couple of weeks to write and validate the fixer,
> hours to run over the tree.
>
> It won't be perfect but will be better than doing it manually.
Here's a thought: perhaps we don't. sysfs_emit isn't a "new api" its a
minor rewrap of existing best practice. The damage caused by the churn
of forcing its use everywhere would far outweigh any actual benefit
because pretty much every bug in this area has already been caught and
killed by existing tools. We can enforce sysfs_emit going forwards
using tools like checkpatch but there's no benefit and a lot of harm to
be done by trying to churn the entire tree retrofitting it (both in
terms of review time wasted as well as patch series derailed).
James
^ permalink raw reply
* Re: [RFC] MAINTAINERS tag for cleanup robot
From: Tom Rix @ 2020-11-22 16:33 UTC (permalink / raw)
To: Joe Perches, clang-built-linux
Cc: linux-hyperv, linux-kernel, xen-devel, tboot-devel, kvm,
linux-crypto, linux-acpi, devel, amd-gfx, dri-devel, intel-gfx,
netdev, linux-media, MPT-FusionLinux.pdl, linux-scsi,
linux-wireless, ibm-acpi-devel, platform-driver-x86, linux-usb,
linux-omap, linux-fbdev, ecryptfs, linux-fsdevel, cluster-devel,
linux-mtd, keyrings, netfilter-devel, coreteam, alsa-devel, bpf,
linux-bluetooth, linux-nfs, patches
In-Reply-To: <2105f0c05e9eae8bee8e17dcc5314474b3c0bc73.camel@perches.com>
On 11/21/20 9:10 AM, Joe Perches wrote:
> On Sat, 2020-11-21 at 08:50 -0800, trix@redhat.com wrote:
>> A difficult part of automating commits is composing the subsystem
>> preamble in the commit log. For the ongoing effort of a fixer producing
>> one or two fixes a release the use of 'treewide:' does not seem appropriate.
>>
>> It would be better if the normal prefix was used. Unfortunately normal is
>> not consistent across the tree.
>>
>> So I am looking for comments for adding a new tag to the MAINTAINERS file
>>
>> D: Commit subsystem prefix
>>
>> ex/ for FPGA DFL DRIVERS
>>
>> D: fpga: dfl:
> I'm all for it. Good luck with the effort. It's not completely trivial.
>
> From a decade ago:
>
> https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/
>
> (and that thread started with extra semicolon patches too)
Reading the history, how about this.
get_mataintainer.pl outputs a single prefix, if multiple files have the same prefix it works, if they don't its an error.
Another script 'commit_one_file.sh' does the call to get_mainainter.pl to get the prefix and be called by run-clang-tools.py to get the fixer specific message.
Defer minimizing the commits by combining similar subsystems till later.
In a steady state case, this should be uncommon.
>
>> Continuing with cleaning up clang's -Wextra-semi-stmt
>> diff --git a/Makefile b/Makefile
> []
>> @@ -1567,20 +1567,21 @@ help:
>> echo ''
>> @echo 'Static analysers:'
>> @echo ' checkstack - Generate a list of stack hogs'
>> @echo ' versioncheck - Sanity check on version.h usage'
>> @echo ' includecheck - Check for duplicate included header files'
>> @echo ' export_report - List the usages of all exported symbols'
>> @echo ' headerdep - Detect inclusion cycles in headers'
>> @echo ' coccicheck - Check with Coccinelle'
>> @echo ' clang-analyzer - Check with clang static analyzer'
>> @echo ' clang-tidy - Check with clang-tidy'
>> + @echo ' clang-tidy-fix - Check and fix with clang-tidy'
> A pity the ordering of the code below isn't the same as the above.
Taken care thanks!
Tom
^ permalink raw reply
* Re: [PATCH 000/141] Fix fall-through warnings for Clang
From: Kees Cook @ 2020-11-22 16:17 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Gustavo A. R. Silva, linux-kernel, alsa-devel, amd-gfx, bridge,
ceph-devel, cluster-devel, coreteam, devel, dm-devel, drbd-dev,
dri-devel, GR-everest-linux-l2, GR-Linux-NIC-Dev, intel-gfx,
intel-wired-lan, keyrings, linux1394-devel, linux-acpi, linux-afs,
linux-arm-kernel, linux-arm-msm, linux-atm-general, linux-block,
linux-can, linux-cifs, linux-crypto, linux-decnet-user,
linux-ext4, linux-fbdev, linux-geode, linux-gpio, linux-hams,
linux-hwmon, linux-i3c, linux-ide, linux-iio, linux-input,
linux-integrity, linux-mediatek, linux-media, linux-mmc, linux-mm,
linux-mtd, linux-nfs, linux-rdma, linux-renesas-soc, linux-scsi,
linux-sctp, linux-security-module, linux-stm32, linux-usb,
linux-watchdog, linux-wireless, netdev, netfilter-devel, nouveau,
op-tee, oss-drivers, patches, rds-devel, reiserfs-devel,
samba-technical, selinux, target-devel, tipc-discussion,
usb-storage, virtualization, wcn36xx, x86, xen-devel,
linux-hardening, Nick Desaulniers, Nathan Chancellor,
Miguel Ojeda, Joe Perches
In-Reply-To: <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote:
> > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote:
> > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:
> > > > This series aims to fix almost all remaining fall-through warnings in
> > > > order to enable -Wimplicit-fallthrough for Clang.
> > > >
> > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly
> > > > add multiple break/goto/return/fallthrough statements instead of just
> > > > letting the code fall through to the next case.
> > > >
> > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this
> > > > change[1] is meant to be reverted at some point. So, this patch helps
> > > > to move in that direction.
> > > >
> > > > Something important to mention is that there is currently a discrepancy
> > > > between GCC and Clang when dealing with switch fall-through to empty case
> > > > statements or to cases that only contain a break/continue/return
> > > > statement[2][3][4].
> > >
> > > Are we sure we want to make this change? Was it discussed before?
> > >
> > > Are there any bugs Clangs puritanical definition of fallthrough helped
> > > find?
> > >
> > > IMVHO compiler warnings are supposed to warn about issues that could
> > > be bugs. Falling through to default: break; can hardly be a bug?!
> >
> > It's certainly a place where the intent is not always clear. I think
> > this makes all the cases unambiguous, and doesn't impact the machine
> > code, since the compiler will happily optimize away any behavioral
> > redundancy.
>
> If none of the 140 patches here fix a real bug, and there is no change
> to machine code then it sounds to me like a W=2 kind of a warning.
FWIW, this series has found at least one bug so far:
https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=Kr_CQ@mail.gmail.com/
--
Kees Cook
^ permalink raw reply
* Re: [RFC] MAINTAINERS tag for cleanup robot
From: Tom Rix @ 2020-11-22 16:10 UTC (permalink / raw)
To: Matthew Wilcox
Cc: joe, clang-built-linux, linux-hyperv, linux-kernel, xen-devel,
tboot-devel, kvm, linux-crypto, linux-acpi, devel, amd-gfx,
dri-devel, intel-gfx, netdev, linux-media, MPT-FusionLinux.pdl,
linux-scsi, linux-wireless, ibm-acpi-devel, platform-driver-x86,
linux-usb, linux-omap, linux-fbdev, ecryptfs, linux-fsdevel,
cluster-devel, linux-mtd, keyrings, netfilter-devel, coreteam,
alsa-devel, bpf, linux-bluetooth, linux-nfs, patches
In-Reply-To: <20201122145635.GG4327@casper.infradead.org>
On 11/22/20 6:56 AM, Matthew Wilcox wrote:
> On Sun, Nov 22, 2020 at 06:46:46AM -0800, Tom Rix wrote:
>> On 11/21/20 7:23 PM, Matthew Wilcox wrote:
>>> On Sat, Nov 21, 2020 at 08:50:58AM -0800, trix@redhat.com wrote:
>>>> The fixer review is
>>>> https://reviews.llvm.org/D91789
>>>>
>>>> A run over allyesconfig for x86_64 finds 62 issues, 5 are false positives.
>>>> The false positives are caused by macros passed to other macros and by
>>>> some macro expansions that did not have an extra semicolon.
>>>>
>>>> This cleans up about 1,000 of the current 10,000 -Wextra-semi-stmt
>>>> warnings in linux-next.
>>> Are any of them not false-positives? It's all very well to enable
>>> stricter warnings, but if they don't fix any bugs, they're just churn.
>>>
>> While enabling additional warnings may be a side effect of this effort
>>
>> the primary goal is to set up a cleaning robot. After that a refactoring robot.
> Why do we need such a thing? Again, it sounds like more churn.
> It's really annoying when I'm working on something important that gets
> derailed by pointless churn. Churn also makes it harder to backport
> patches to earlier kernels.
>
A refactoring example on moving to treewide, consistent use of a new api may help.
Consider
2efc459d06f1630001e3984854848a5647086232
sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output
A new api for printing in the sysfs. How do we use it treewide ?
Done manually, it would be a heroic effort requiring high level maintainers pushing and likely only get partially done.
If a refactoring programatic fixit is done and validated on a one subsystem, it can run on all the subsystems.
The effort is a couple of weeks to write and validate the fixer, hours to run over the tree.
It won't be perfect but will be better than doing it manually.
Tom
^ permalink raw reply
* Re: [PATCH mlx5-next 09/16] net/mlx5: Expose IP-in-IP TX and RX capability bits
From: Aya Levin @ 2020-11-22 15:17 UTC (permalink / raw)
To: Jakub Kicinski, Saeed Mahameed
Cc: Leon Romanovsky, netdev, linux-rdma, Moshe Shemesh
In-Reply-To: <20201121155852.4ca8eb68@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On 11/22/2020 1:58 AM, Jakub Kicinski wrote:
> On Fri, 20 Nov 2020 15:03:32 -0800 Saeed Mahameed wrote:
>> From: Aya Levin <ayal@nvidia.com>
>>
>> Expose FW indication that it supports stateless offloads for IP over IP
>> tunneled packets per direction. In some HW like ConnectX-4 IP-in-IP
>> support is not symmetric, it supports steering on the inner header but
>> it doesn't TX-Checksum and TSO. Add IP-in-IP capability per direction to
>> cover this case as well.
>
> What's the use for the rx capability in Linux? We don't have an API to
> configure that AFAIK.
>
Correct, the rx capability bit is used by the driver to allow flow
steering on the inner header.
^ permalink raw reply
* Re: [PATCH net 02/15] ibmvnic: process HMC disable command
From: drt @ 2020-11-22 15:12 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Lijun Pan, netdev, sukadev, drt
In-Reply-To: <20201121153637.17a91ac4@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On 2020-11-21 15:36, 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?
This allows HMC to disconnect a Linux client from the network if the
vNIC adapter is misbehaving and/or sending malicious traffic. The effect
is the same as when a sysadmin sets a link down (ibmvnic_close()) on the
Linux client. This patch extends this ability to the HMC.
Thanks!
Dany
^ permalink raw reply
* Re: [RFC] MAINTAINERS tag for cleanup robot
From: Matthew Wilcox @ 2020-11-22 14:56 UTC (permalink / raw)
To: Tom Rix
Cc: joe, clang-built-linux, linux-hyperv, linux-kernel, xen-devel,
tboot-devel, kvm, linux-crypto, linux-acpi, devel, amd-gfx,
dri-devel, intel-gfx, netdev, linux-media, MPT-FusionLinux.pdl,
linux-scsi, linux-wireless, ibm-acpi-devel, platform-driver-x86,
linux-usb, linux-omap, linux-fbdev, ecryptfs, linux-fsdevel,
cluster-devel, linux-mtd, keyrings, netfilter-devel, coreteam,
alsa-devel, bpf, linux-bluetooth, linux-nfs, patches
In-Reply-To: <ddb08a27-3ca1-fb2e-d51f-4b471f1a56a3@redhat.com>
On Sun, Nov 22, 2020 at 06:46:46AM -0800, Tom Rix wrote:
>
> On 11/21/20 7:23 PM, Matthew Wilcox wrote:
> > On Sat, Nov 21, 2020 at 08:50:58AM -0800, trix@redhat.com wrote:
> >> The fixer review is
> >> https://reviews.llvm.org/D91789
> >>
> >> A run over allyesconfig for x86_64 finds 62 issues, 5 are false positives.
> >> The false positives are caused by macros passed to other macros and by
> >> some macro expansions that did not have an extra semicolon.
> >>
> >> This cleans up about 1,000 of the current 10,000 -Wextra-semi-stmt
> >> warnings in linux-next.
> > Are any of them not false-positives? It's all very well to enable
> > stricter warnings, but if they don't fix any bugs, they're just churn.
> >
> While enabling additional warnings may be a side effect of this effort
>
> the primary goal is to set up a cleaning robot. After that a refactoring robot.
Why do we need such a thing? Again, it sounds like more churn.
It's really annoying when I'm working on something important that gets
derailed by pointless churn. Churn also makes it harder to backport
patches to earlier kernels.
^ permalink raw reply
* Re: [PATCH net-next,v5 0/9] netfilter: flowtable bridge and vlan enhancements
From: Alexander Lobakin @ 2020-11-22 14:51 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Alexander Lobakin, netfilter-devel, davem, netdev, kuba, fw,
razor, jeremy, tobias, linux-kernel
In-Reply-To: <20201122102605.2342-1-alobakin@pm.me>
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Sun, 22 Nov 2020 12:42:19 +0100
> On Sun, Nov 22, 2020 at 10:26:16AM +0000, Alexander Lobakin wrote:
>> From: Pablo Neira Ayuso <pablo@netfilter.org>
>> Date: Fri, 20 Nov 2020 13:49:12 +0100
> [...]
>>> Something like this:
>>>
>>> fast path
>>> .------------------------.
>>> / \
>>> | IP forwarding |
>>> | / \ .
>>> | br0 eth0
>>> . / \
>>> -- veth1 veth2
>>> .
>>> .
>>> .
>>> eth0
>>> ab:cd:ef:ab:cd:ef
>>> VM
>>
>> I'm concerned about bypassing vlan and bridge's .ndo_start_xmit() in
>> case of this shortcut. We'll have incomplete netdevice Tx stats for
>> these two, as it gets updated inside this callbacks.
>
> TX device stats are being updated accordingly.
>
> # ip netns exec nsr1 ip -s link
> 1: lo: <LOOPBACK> mtu 65536 qdisc noop state DOWN mode DEFAULT group default qlen 1000
> link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
> RX: bytes packets errors dropped overrun mcast
> 0 0 0 0 0 0
> TX: bytes packets errors dropped carrier collsns
> 0 0 0 0 0 0
> 2: veth0@if2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> link/ether 82:0d:f3:b5:59:5d brd ff:ff:ff:ff:ff:ff link-netns ns1
> RX: bytes packets errors dropped overrun mcast
> 213290848248 4869765 0 0 0 0
> TX: bytes packets errors dropped carrier collsns
> 315346667 4777953 0 0 0 0
> 3: veth1@if2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> link/ether 4a:81:2d:9a:02:88 brd ff:ff:ff:ff:ff:ff link-netns ns2
> RX: bytes packets errors dropped overrun mcast
> 315337919 4777833 0 0 0 0
> TX: bytes packets errors dropped carrier collsns
> 213290844826 4869708 0 0 0 0
> 4: br0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> link/ether 82:0d:f3:b5:59:5d brd ff:ff:ff:ff:ff:ff
> RX: bytes packets errors dropped overrun mcast
> 4101 73 0 0 0 0
> TX: bytes packets errors dropped carrier collsns
> 5256 74 0 0 0 0
Aren't these counters very low for br0, despite that br0 is an
intermediate point of traffic flow?
> 5: veth0.10@veth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br0 state UP mode DEFAULT group default qlen 1000
> link/ether 82:0d:f3:b5:59:5d brd ff:ff:ff:ff:ff:ff
> RX: bytes packets errors dropped overrun mcast
> 4101 73 0 0 0 62
> TX: bytes packets errors dropped carrier collsns
> 315342363 4777893 0 0 0 0
^ permalink raw reply
* Re: [RFC] MAINTAINERS tag for cleanup robot
From: Tom Rix @ 2020-11-22 14:46 UTC (permalink / raw)
To: Matthew Wilcox
Cc: joe, clang-built-linux, linux-hyperv, linux-kernel, xen-devel,
tboot-devel, kvm, linux-crypto, linux-acpi, devel, amd-gfx,
dri-devel, intel-gfx, netdev, linux-media, MPT-FusionLinux.pdl,
linux-scsi, linux-wireless, ibm-acpi-devel, platform-driver-x86,
linux-usb, linux-omap, linux-fbdev, ecryptfs, linux-fsdevel,
cluster-devel, linux-mtd, keyrings, netfilter-devel, coreteam,
alsa-devel, bpf, linux-bluetooth, linux-nfs, patches
In-Reply-To: <20201122032304.GE4327@casper.infradead.org>
On 11/21/20 7:23 PM, Matthew Wilcox wrote:
> On Sat, Nov 21, 2020 at 08:50:58AM -0800, trix@redhat.com wrote:
>> The fixer review is
>> https://reviews.llvm.org/D91789
>>
>> A run over allyesconfig for x86_64 finds 62 issues, 5 are false positives.
>> The false positives are caused by macros passed to other macros and by
>> some macro expansions that did not have an extra semicolon.
>>
>> This cleans up about 1,000 of the current 10,000 -Wextra-semi-stmt
>> warnings in linux-next.
> Are any of them not false-positives? It's all very well to enable
> stricter warnings, but if they don't fix any bugs, they're just churn.
>
While enabling additional warnings may be a side effect of this effort
the primary goal is to set up a cleaning robot. After that a refactoring robot.
Tom
^ permalink raw reply
* Re: [PATCH net-next 2/3] mlxsw: spectrum_ptp: use PTP wide message type definitions
From: Ido Schimmel @ 2020-11-22 14:35 UTC (permalink / raw)
To: Christian Eggers
Cc: Richard Cochran, Andrew Lunn, Heiner Kallweit, Jakub Kicinski,
Vladimir Oltean, Russell King, David S . Miller, netdev,
linux-kernel, Christian Eggers, Petr Machata, Jiri Pirko,
Ido Schimmel
In-Reply-To: <20201122082636.12451-3-ceggers@arri.de>
On Sun, Nov 22, 2020 at 09:26:35AM +0100, Christian Eggers wrote:
> Use recently introduced PTP wide defines instead of a driver internal
> enumeration.
>
> Signed-off-by: Christian Eggers <ceggers@gmx.de>
> Cc: Petr Machata <petrm@mellanox.com>
> Cc: Jiri Pirko <jiri@nvidia.com>
> Cc: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
But:
1. Checkpatch complains about:
WARNING: From:/Signed-off-by: email address mismatch: 'From: Christian Eggers <ceggers@arri.de>' != 'Signed-off-by: Christian Eggers <ceggers@gmx.de>'
2. This series does not build, which fails the CI [1][2] and also
required me to fetch the dependencies that are currently under review
[3]. I believe it is generally discouraged to create dependencies
between patch sets that are under review for exactly these reasons. I
don't know what are Jakub's preferences, but had this happened on our
internal patchwork instance, I would just ask the author to submit
another version with all the patches.
Anyway, I added all six patches to our regression as we have some PTP
tests. Will let you know tomorrow.
Thanks
[1] https://lore.kernel.org/netdev/20201122082636.12451-1-ceggers@arri.de/T/#mcef35858585d23b72b8f75450a51618d5c5d3260
[2] https://patchwork.hopto.org/static/nipa/389053/11923809/build_allmodconfig_warn/summary
[3] https://patchwork.kernel.org/project/netdevbpf/cover/20201120084106.10046-1-ceggers@arri.de/
^ permalink raw reply
* Re: general protection fault in ieee80211_chanctx_num_assigned
From: syzbot @ 2020-11-22 13:56 UTC (permalink / raw)
To: davem, johannes, kuba, linux-kernel, linux-wireless, netdev,
syzkaller-bugs
In-Reply-To: <00000000000086205205b0fff8b2@google.com>
syzbot has found a reproducer for the following issue on:
HEAD commit: a349e4c6 Merge tag 'xfs-5.10-fixes-7' of git://git.kernel...
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=144e1e99500000
kernel config: https://syzkaller.appspot.com/x/.config?x=330f3436df12fd44
dashboard link: https://syzkaller.appspot.com/bug?extid=00ce7332120071df39b1
compiler: gcc (GCC) 10.1.0-syz 20200507
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=153140a5500000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=179bf835500000
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+00ce7332120071df39b1@syzkaller.appspotmail.com
general protection fault, probably for non-canonical address 0xfbd59c0000000020: 0000 [#1] PREEMPT SMP KASAN
KASAN: maybe wild-memory-access in range [0xdead000000000100-0xdead000000000107]
CPU: 1 PID: 8531 Comm: syz-executor169 Not tainted 5.10.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:ieee80211_chanctx_num_assigned+0xb1/0x140 net/mac80211/chan.c:21
Code: a8 f6 ff ff 48 39 c5 74 3b 49 bd 00 00 00 00 00 fc ff df e8 c1 91 1b f9 48 8d bb 58 09 00 00 41 83 c4 01 48 89 f8 48 c1 e8 03 <42> 80 3c 28 00 75 68 48 8b 83 58 09 00 00 48 8d 98 a8 f6 ff ff 48
RSP: 0018:ffffc9000169f330 EFLAGS: 00010a02
RAX: 1bd5a00000000020 RBX: deacfffffffff7a8 RCX: ffffffff88549e6b
RDX: ffff888011c8b480 RSI: ffffffff88549e0f RDI: dead000000000100
RBP: ffff8880130ca720 R08: 0000000000000000 R09: ffffffff8cecb9cf
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000002
R13: dffffc0000000000 R14: ffff8880130ca700 R15: 0000000000000000
FS: 000000000087d940(0000) GS:ffff8880b9f00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000006d3090 CR3: 000000001c20a000 CR4: 00000000001506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
ieee80211_assign_vif_chanctx+0x7b8/0x1230 net/mac80211/chan.c:690
__ieee80211_vif_release_channel+0x236/0x430 net/mac80211/chan.c:1557
ieee80211_vif_release_channel+0x117/0x220 net/mac80211/chan.c:1771
ieee80211_ibss_disconnect+0x44e/0x7b0 net/mac80211/ibss.c:735
ieee80211_ibss_leave+0x12/0xe0 net/mac80211/ibss.c:1871
rdev_leave_ibss net/wireless/rdev-ops.h:545 [inline]
__cfg80211_leave_ibss+0x19a/0x4c0 net/wireless/ibss.c:212
cfg80211_leave_ibss+0x57/0x80 net/wireless/ibss.c:230
cfg80211_change_iface+0x855/0xef0 net/wireless/util.c:1012
nl80211_set_interface+0x65c/0x8d0 net/wireless/nl80211.c:3789
genl_family_rcv_msg_doit+0x228/0x320 net/netlink/genetlink.c:739
genl_family_rcv_msg net/netlink/genetlink.c:783 [inline]
genl_rcv_msg+0x328/0x580 net/netlink/genetlink.c:800
netlink_rcv_skb+0x153/0x420 net/netlink/af_netlink.c:2494
genl_rcv+0x24/0x40 net/netlink/genetlink.c:811
netlink_unicast_kernel net/netlink/af_netlink.c:1304 [inline]
netlink_unicast+0x533/0x7d0 net/netlink/af_netlink.c:1330
netlink_sendmsg+0x856/0xd90 net/netlink/af_netlink.c:1919
sock_sendmsg_nosec net/socket.c:651 [inline]
sock_sendmsg+0xcf/0x120 net/socket.c:671
____sys_sendmsg+0x6e8/0x810 net/socket.c:2353
___sys_sendmsg+0xf3/0x170 net/socket.c:2407
__sys_sendmsg+0xe5/0x1b0 net/socket.c:2440
do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x4429b9
Code: e8 bc fd 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 db 06 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffd820d0a58 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00000000004429b9
RDX: 0000000000000000 RSI: 0000000020000340 RDI: 0000000000000004
RBP: 000000000000fbef R08: 00000000004035b0 R09: 00000000004035b0
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000403520
R13: 00000000004035b0 R14: 0000000000000000 R15: 0000000000000000
Modules linked in:
---[ end trace 4cedfcb59a8efe47 ]---
RIP: 0010:ieee80211_chanctx_num_assigned+0xb1/0x140 net/mac80211/chan.c:21
Code: a8 f6 ff ff 48 39 c5 74 3b 49 bd 00 00 00 00 00 fc ff df e8 c1 91 1b f9 48 8d bb 58 09 00 00 41 83 c4 01 48 89 f8 48 c1 e8 03 <42> 80 3c 28 00 75 68 48 8b 83 58 09 00 00 48 8d 98 a8 f6 ff ff 48
RSP: 0018:ffffc9000169f330 EFLAGS: 00010a02
RAX: 1bd5a00000000020 RBX: deacfffffffff7a8 RCX: ffffffff88549e6b
RDX: ffff888011c8b480 RSI: ffffffff88549e0f RDI: dead000000000100
RBP: ffff8880130ca720 R08: 0000000000000000 R09: ffffffff8cecb9cf
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000002
R13: dffffc0000000000 R14: ffff8880130ca700 R15: 0000000000000000
FS: 000000000087d940(0000) GS:ffff8880b9e00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007efedefa7000 CR3: 000000001c20a000 CR4: 00000000001506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
^ permalink raw reply
* [PATCH] libbpf: add support for canceling cached_cons advance
From: Li RongQing @ 2020-11-22 13:10 UTC (permalink / raw)
To: netdev, bpf
It is possible to fail receiving packets after calling
xsk_ring_cons__peek, at this condition, cached_cons has
been advanced, should be cancelled.
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
tools/lib/bpf/xsk.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
index 1069c46364ff..4128215c246b 100644
--- a/tools/lib/bpf/xsk.h
+++ b/tools/lib/bpf/xsk.h
@@ -153,6 +153,12 @@ static inline size_t xsk_ring_cons__peek(struct xsk_ring_cons *cons,
return entries;
}
+static inline void xsk_ring_cons__cancel(struct xsk_ring_cons *cons,
+ size_t nb)
+{
+ rx->cached_cons -= nb;
+}
+
static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, size_t nb)
{
/* Make sure data has been read before indicating we are done
--
2.17.3
^ permalink raw reply related
* Re: [PATCH net-next 0/2] TLS TX HW offload for Bond
From: Tariq Toukan @ 2020-11-22 12:48 UTC (permalink / raw)
To: Jakub Kicinski, Jarod Wilson
Cc: Tariq Toukan, David S. Miller, netdev, Saeed Mahameed,
Moshe Shemesh, Maor Gottlieb
In-Reply-To: <20201119083811.6b68bfa8@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
On 11/19/2020 6:38 PM, Jakub Kicinski wrote:
> On Thu, 19 Nov 2020 17:59:38 +0200 Tariq Toukan wrote:
>> On 11/19/2020 2:02 AM, Jakub Kicinski wrote:
>>> On Sun, 15 Nov 2020 15:42:49 +0200 Tariq Toukan wrote:
>>>> This series opens TLS TX HW offload for bond interfaces.
>>>> This allows bond interfaces to benefit from capable slave devices.
>>>>
>>>> The first patch adds real_dev field in TLS context structure, and aligns
>>>> usages in TLS module and supporting drivers.
>>>> The second patch opens the offload for bond interfaces.
>>>>
>>>> For the configuration above, SW kTLS keeps picking the same slave
>>>> To keep simple track of the HW and SW TLS contexts, we bind each socket to
>>>> a specific slave for the socket's whole lifetime. This is logically valid
>>>> (and similar to the SW kTLS behavior) in the following bond configuration,
>>>> so we restrict the offload support to it:
>>>>
>>>> ((mode == balance-xor) or (mode == 802.3ad))
>>>> and xmit_hash_policy == layer3+4.
>>>
>>> This does not feel extremely clean, maybe you can convince me otherwise.
>>>
>>> Can we extend netdev_get_xmit_slave() and figure out the output dev
>>> (and if it's "stable") in a more generic way? And just feed that dev
>>> into TLS handling?
>>
>> I don't see we go through netdev_get_xmit_slave(), but through
>> .ndo_start_xmit (bond_start_xmit).
>
> I may be misunderstanding the purpose of netdev_get_xmit_slave(),
> please correct me if I'm wrong. AFAIU it's supposed to return a
> lower netdev that the skb should then be xmited on.
That's true. It was recently added and used by the RDMA team. Not used
or integrated in the Eth networking stack.
> So what I was thinking was either construct an skb or somehow reshuffle
> the netdev_get_xmit_slave() code to take a flow dissector output or
> ${insert other ideas}. Then add a helper in the core that would drill
> down from the socket netdev to the "egress" netdev. Have TLS call
> that helper, and talk to the "egress" netdev from the start, rather
> than the socket's netdev. Then loosen the checks on software devices.
As I understand it, best if we can even generalize this to apply to all
kinds of traffic: bond driver won't do the xmit itself anymore, it just
picks an egress dev and returns it. The core infrastructure will call
the xmit function for the egress dev.
I like the idea, it can generalize code structures for all kinds of
upper-devices and sockets, taking them into a common place in core,
which reduces code duplications.
If we go only half the way, i.e. keep xmit logic in bond for
non-TLS-offloaded traffic, then we have to let TLS module (and others in
the future) act deferentially for different kinds of devs (upper/lower)
which IMHO reduces generality.
I'm in favor of the deeper change. It will be on a larger scale, and
totally orthogonal to the current TLS offload support in bond.
If we decide to apply the idea only to TLS sockets (or any subset of
sockets) we're actually taking a generic one-flow (the xmit patch of a
bond dev) and turning it into two (or potentially more) flows, depending
on the socket type. This also reduces generality.
>
> I'm probably missing the problem you're trying to explain to me :S
>
I kept the patch minimal, and kept the TLS offload logic internal to the
bond driver, just like it is internal to the device drivers (mlx5e, and
others), with no core infrastructure modification.
> Side note - Jarod, I'd be happy to take a patch renaming
> netdev_get_xmit_slave() and the ndo, if you have the cycles to send
> a patch. It's a recent addition, and in the core we should make more
> of an effort to avoid sensitive terms.
>
>> Currently I have my check there to
>> catch all skbs belonging to offloaded TLS sockets.
>>
>> The TLS offload get_slave() logic decision is per socket, so the result
>> cannot be saved in the bond memory. Currently I save the real_dev field
>> in the TLS context structure.
>
> Right, but we could just have ctx->netdev be the "egress" netdev
> always, right? Do we expect somewhere that it's going to be matching
> the socket's dst?
>
So once the offload context is established we totally bypass the bond
dev? and lose interaction or reference to it?
What if the egress dev is detached form the bond? We must then be
notified somehow.
>> One way to make it more generic is to save it on the sock structure. I
>> agree that this replaces the TLS-specific logic, but demands increasing
>> the sock struct, and has larger impact on all other flows...
>
^ permalink raw reply
* Re: [PATCH] ipvs: replace atomic_add_return()
From: Pablo Neira Ayuso @ 2020-11-22 12:46 UTC (permalink / raw)
To: Julian Anastasov
Cc: Yejune Deng, wensong, horms, kadlec, fw, davem, kuba, netdev,
lvs-devel, netfilter-devel, linux-kernel
In-Reply-To: <9cd77e1e-1c52-d647-9443-485510b4a9b1@ssi.bg>
On Tue, Nov 17, 2020 at 10:57:52PM +0200, Julian Anastasov wrote:
>
> Hello,
>
> On Mon, 16 Nov 2020, Yejune Deng wrote:
>
> > atomic_inc_return() looks better
> >
> > Signed-off-by: Yejune Deng <yejune.deng@gmail.com>
>
> Looks good to me for -next, thanks!
>
> Acked-by: Julian Anastasov <ja@ssi.bg>
Applied, thanks.
^ 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