Re: [PATCH] xen/gntalloc: Replace UAPI 1-element array
On 2/6/24 11:03, Kees Cook wrote:
Without changing the structure size (since it is UAPI), add a proper
flexible array member, and reference it in the kernel so that it will
not be trip the array-bounds sanitizer[1].
Link: https://github.com/KSPP/linux/issues/113 [1]
Cc: Juergen Gross
Cc: Stefano Stabellini
Cc: Oleksandr Tyshchenko
Cc: Gustavo A. R. Silva
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Kees Cook
Reviewed-by: Gustavo A. R. Silva
Thanks!
--
Gustavo
---
drivers/xen/gntalloc.c | 2 +-
include/uapi/xen/gntalloc.h | 5 -
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 26ffb8755ffb..f93f73ecefee 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -317,7 +317,7 @@ static long gntalloc_ioctl_alloc(struct
gntalloc_file_private_data *priv,
rc = -EFAULT;
goto out_free;
}
- if (copy_to_user(arg->gref_ids, gref_ids,
+ if (copy_to_user(arg->gref_ids_flex, gref_ids,
sizeof(gref_ids[0]) * op.count)) {
rc = -EFAULT;
goto out_free;
diff --git a/include/uapi/xen/gntalloc.h b/include/uapi/xen/gntalloc.h
index 48d2790ef928..3109282672f3 100644
--- a/include/uapi/xen/gntalloc.h
+++ b/include/uapi/xen/gntalloc.h
@@ -31,7 +31,10 @@ struct ioctl_gntalloc_alloc_gref {
__u64 index;
/* The grant references of the newly created grant, one per page */
/* Variable size, depending on count */
- __u32 gref_ids[1];
+ union {
+ __u32 gref_ids[1];
+ __DECLARE_FLEX_ARRAY(__u32, gref_ids_flex);
+ };
};
#define GNTALLOC_FLAG_WRITABLE 1
Re: [PATCH][next] xen: privcmd: Replace zero-length array with flex-array member and use __counted_by
On 11/16/23 15:08, Kees Cook wrote: On Thu, Nov 16, 2023 at 12:54:59PM -0600, Gustavo A. R. Silva wrote: Fake flexible arrays (zero-length and one-element arrays) are deprecated, and should be replaced by flexible-array members. So, replace zero-length array with a flexible-array member in `struct privcmd_kernel_ioreq`. Also annotate array `ports` with `__counted_by()` to prepare for the coming implementation by GCC and Clang of the `__counted_by` attribute. Flexible array members annotated with `__counted_by` can have their accesses bounds-checked at run-time via `CONFIG_UBSAN_BOUNDS` (for array indexing) and `CONFIG_FORTIFY_SOURCE` (for strcpy/memcpy-family functions). This fixes multiple -Warray-bounds warnings: drivers/xen/privcmd.c:1239:30: warning: array subscript i is outside array bounds of 'struct ioreq_port[0]' [-Warray-bounds=] drivers/xen/privcmd.c:1240:30: warning: array subscript i is outside array bounds of 'struct ioreq_port[0]' [-Warray-bounds=] drivers/xen/privcmd.c:1241:30: warning: array subscript i is outside array bounds of 'struct ioreq_port[0]' [-Warray-bounds=] drivers/xen/privcmd.c:1245:33: warning: array subscript i is outside array bounds of 'struct ioreq_port[0]' [-Warray-bounds=] drivers/xen/privcmd.c:1258:67: warning: array subscript i is outside array bounds of 'struct ioreq_port[0]' [-Warray-bounds=] This results in no differences in binary output. Signed-off-by: Gustavo A. R. Silva Looks right to me. I can see the allocation: Yep, I always check for that; in particular, the 'counter' assignment. :) Do you want me to mention it in the changelog text? size = struct_size(kioreq, ports, ioeventfd->vcpus); kioreq = kzalloc(size, GFP_KERNEL); if (!kioreq) return ERR_PTR(-ENOMEM); kioreq->dom = ioeventfd->dom; kioreq->vcpus = ioeventfd->vcpus; Reviewed-by: Kees Cook Thanks! -- Gustavo
[PATCH][next] xen: privcmd: Replace zero-length array with flex-array member and use __counted_by
Fake flexible arrays (zero-length and one-element arrays) are deprecated,
and should be replaced by flexible-array members. So, replace
zero-length array with a flexible-array member in `struct
privcmd_kernel_ioreq`.
Also annotate array `ports` with `__counted_by()` to prepare for the
coming implementation by GCC and Clang of the `__counted_by` attribute.
Flexible array members annotated with `__counted_by` can have their
accesses bounds-checked at run-time via `CONFIG_UBSAN_BOUNDS` (for array
indexing) and `CONFIG_FORTIFY_SOURCE` (for strcpy/memcpy-family functions).
This fixes multiple -Warray-bounds warnings:
drivers/xen/privcmd.c:1239:30: warning: array subscript i is outside array
bounds of 'struct ioreq_port[0]' [-Warray-bounds=]
drivers/xen/privcmd.c:1240:30: warning: array subscript i is outside array
bounds of 'struct ioreq_port[0]' [-Warray-bounds=]
drivers/xen/privcmd.c:1241:30: warning: array subscript i is outside array
bounds of 'struct ioreq_port[0]' [-Warray-bounds=]
drivers/xen/privcmd.c:1245:33: warning: array subscript i is outside array
bounds of 'struct ioreq_port[0]' [-Warray-bounds=]
drivers/xen/privcmd.c:1258:67: warning: array subscript i is outside array
bounds of 'struct ioreq_port[0]' [-Warray-bounds=]
This results in no differences in binary output.
Signed-off-by: Gustavo A. R. Silva
---
drivers/xen/privcmd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 1ce7f3c7a950..0eb337a8ec0f 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -1115,7 +1115,7 @@ struct privcmd_kernel_ioreq {
spinlock_t lock; /* Protects ioeventfds list */
struct list_head ioeventfds;
struct list_head list;
- struct ioreq_port ports[0];
+ struct ioreq_port ports[] __counted_by(vcpus);
};
static irqreturn_t ioeventfd_interrupt(int irq, void *dev_id)
--
2.34.1
[PATCH][next] xen/xenbus: Add __counted_by for struct read_buffer and use struct_size()
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for
array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
While there, use struct_size() helper, instead of the open-coded
version, to calculate the size for the allocation of the whole
flexible structure, including of course, the flexible-array member.
This code was found with the help of Coccinelle, and audited and
fixed manually.
Signed-off-by: Gustavo A. R. Silva
---
drivers/xen/xenbus/xenbus_dev_frontend.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c
b/drivers/xen/xenbus/xenbus_dev_frontend.c
index 0792fda49a15..6f56640092a9 100644
--- a/drivers/xen/xenbus/xenbus_dev_frontend.c
+++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
@@ -82,7 +82,7 @@ struct read_buffer {
struct list_head list;
unsigned int cons;
unsigned int len;
- char msg[];
+ char msg[] __counted_by(len);
};
struct xenbus_file_priv {
@@ -195,7 +195,7 @@ static int queue_reply(struct list_head *queue, const void
*data, size_t len)
if (len > XENSTORE_PAYLOAD_MAX)
return -EINVAL;
- rb = kmalloc(sizeof(*rb) + len, GFP_KERNEL);
+ rb = kmalloc(struct_size(rb, msg, len), GFP_KERNEL);
if (rb == NULL)
return -ENOMEM;
--
2.34.1
[PATCH][next] xen: Replace one-element array with flexible-array member
One-element arrays are deprecated, and we are replacing them with flexible
array members instead. So, replace one-element array with flexible-array
member in struct xen_page_directory.
This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
routines on memcpy() and help us make progress towards globally
enabling -fstrict-flex-arrays=3 [1].
This results in no differences in binary output.
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/255
Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
Signed-off-by: Gustavo A. R. Silva
---
drivers/xen/xen-front-pgdir-shbuf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/xen/xen-front-pgdir-shbuf.c
b/drivers/xen/xen-front-pgdir-shbuf.c
index 5c0b5cb5b419..b52e0fa595a9 100644
--- a/drivers/xen/xen-front-pgdir-shbuf.c
+++ b/drivers/xen/xen-front-pgdir-shbuf.c
@@ -30,7 +30,7 @@
struct xen_page_directory {
grant_ref_t gref_dir_next_page;
#define XEN_GREF_LIST_END 0
- grant_ref_t gref[1]; /* Variable length */
+ grant_ref_t gref[]; /* Variable length */
};
/**
--
2.34.1
Re: [PATCH 28/32] selinux: Use mem_to_flex_dup() with xfrm and sidtab
On Thu, May 05, 2022 at 07:16:18PM -0400, Paul Moore wrote:
> On Thu, May 5, 2022 at 2:39 PM Kees Cook wrote:
> > On Wed, May 04, 2022 at 11:14:42PM -0400, Paul Moore wrote:
> > > On Wed, May 4, 2022 at 7:34 PM Gustavo A. R. Silva
> > > wrote:
> > > >
> > > > Hi Paul,
> > > >
> > > > On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote:
> > > > > On Tue, May 3, 2022 at 9:57 PM Kees Cook
> > > > > wrote:
> > > >
> > > > [..]
> > > >
> > > > > > +++ b/include/uapi/linux/xfrm.h
> > > > > > @@ -31,9 +31,9 @@ struct xfrm_id {
> > > > > > struct xfrm_sec_ctx {
> > > > > > __u8ctx_doi;
> > > > > > __u8ctx_alg;
> > > > > > - __u16 ctx_len;
> > > > > > + __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u16, ctx_len);
> > > > > > __u32 ctx_sid;
> > > > > > - charctx_str[0];
> > > > > > + __DECLARE_FLEX_ARRAY_ELEMENTS(char, ctx_str);
> > > > > > };
> > > > >
> > > > > While I like the idea of this in principle, I'd like to hear about the
> > > > > testing you've done on these patches. A previous flex array
> > > > > conversion in the audit uapi headers ended up causing a problem with
> > > >
> > > > I'm curious about which commit caused those problems...?
> > >
> > > Commit ed98ea2128b6 ("audit: replace zero-length array with
> > > flexible-array member"), however, as I said earlier, the problem was
> > > actually with SWIG, it just happened to be triggered by the kernel
> > > commit. There was a brief fedora-devel mail thread about the problem,
> > > see the link below:
> > >
> > > * https://www.spinics.net/lists/fedora-devel/msg297991.html
> >
> > Wow, that's pretty weird -- it looks like SWIG was scraping the headers
> > to build its conversions? I assume SWIG has been fixed now?
>
> I honestly don't know, the audit userspace was hacking around it with
> some header file duplication/munging last I heard, but I try to avoid
> having to touch Steve's audit userspace code.
>
> > > To reiterate, I'm supportive of changes like this, but I would like to
> > > hear how it was tested to ensure there are no unexpected problems with
> > > userspace. If there are userspace problems it doesn't mean we can't
> > > make changes like this, it just means we need to ensure that the
> > > userspace issues are resolved first.
> >
> > Well, as this is the first and only report of any problems with [0] -> []
> > conversions (in UAPI or anywhere) that I remember seeing, and they've
> > been underway since at least v5.9, I hadn't been doing any new testing.
>
> ... and for whatever it is worth, I wasn't expecting it to be a
> problem either. Surprise :)
>
> > So, for this case, I guess I should ask what tests you think would be
> > meaningful here? Anything using #include should be fine:
> > https://codesearch.debian.net/search?q=linux%2Fxfrm.h=1=1
> > Which leaves just this, which may be doing something weird:
> >
> > libabigail_2.0-1/tests/data/test-diff-filter/test-PR27569-v0.abi
> >
> >
> >> filepath="include/uapi/linux/xfrm.h" line="97" column="1"/>
> >
> >
> >
> > But I see that SWIG doesn't show up in a search for linux/audit.h:
> > https://codesearch.debian.net/search?q=linux%2Faudit.h=1=1
> >
> > So this may not be a sufficient analysis...
>
> I think from a practical perspective ensuring that the major IPsec/IKE
> tools, e.g. the various *SWANs, that know about labeled IPSec still
> build and can set/get the SA/SPD labels correctly would be sufficient.
> I seriously doubt there would be any problems, but who knows.
There are certainly some cases in which the transformation of
zero-length arrays into flexible-array members can bring some issues
to the surface[1][2]. This is the first time that we know of one of
them in user-space. However, we haven't transformed the arrays in
UAPI yet (with the exception of a couple of cases[3][4]). But that
is something that we are planning to try soon[5].
--
Gustavo
[1] https://github.com/KSPP/linux/issues?q=invalid+use+of+flexible+array
[2]
https://github.com/KSPP/linux/issues?q=invalid+application+of+%E2%80%98sizeof%E2%80%99+to+incomplete+type
[3] https://git.kernel.org/linus/db243b796439c0caba47865564d8acd18a301d18
[4] https://git.kernel.org/linus/d6cdad870358128c1e753e6258e295ab8a5a2429
[5]
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=for-next/kspp-fam0-uapi
Re: [PATCH 28/32] selinux: Use mem_to_flex_dup() with xfrm and sidtab
Hi Paul,
On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote:
> On Tue, May 3, 2022 at 9:57 PM Kees Cook wrote:
[..]
> > +++ b/include/uapi/linux/xfrm.h
> > @@ -31,9 +31,9 @@ struct xfrm_id {
> > struct xfrm_sec_ctx {
> > __u8ctx_doi;
> > __u8ctx_alg;
> > - __u16 ctx_len;
> > + __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u16, ctx_len);
> > __u32 ctx_sid;
> > - charctx_str[0];
> > + __DECLARE_FLEX_ARRAY_ELEMENTS(char, ctx_str);
> > };
>
> While I like the idea of this in principle, I'd like to hear about the
> testing you've done on these patches. A previous flex array
> conversion in the audit uapi headers ended up causing a problem with
I'm curious about which commit caused those problems...?
Thanks
--
Gustavo
> GCC12 and SWIG; while it was a SWIG problem and not a kernel header
> problem that was thin consolation for those with broken builds.
Re: [PATCH 01/32] netlink: Avoid memcpy() across flexible array boundary
On Tue, May 03, 2022 at 06:44:10PM -0700, Kees Cook wrote:
> In preparation for run-time memcpy() bounds checking, split the nlmsg
> copying for error messages (which crosses a previous unspecified flexible
> array boundary) in half. Avoids the future run-time warning:
>
> memcpy: detected field-spanning write (size 32) of single field
> ">msg" (size 16)
>
> Creates an explicit flexible array at the end of nlmsghdr for the payload,
> named "nlmsg_payload". There is no impact on UAPI; the sizeof(struct
> nlmsghdr) does not change, but now the compiler can better reason about
> where things are being copied.
>
> Fixed-by: Rasmus Villemoes
> Link:
> https://lore.kernel.org/lkml/d7251d92-150b-5346-6237-52afc154b...@rasmusvillemoes.dk
> Cc: "David S. Miller"
> Cc: Jakub Kicinski
> Cc: Rich Felker
> Cc: Eric Dumazet
> Cc: net...@vger.kernel.org
> Signed-off-by: Kees Cook
> ---
> include/uapi/linux/netlink.h | 1 +
> net/netlink/af_netlink.c | 5 -
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> index 855dffb4c1c3..47f9342d51bc 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -47,6 +47,7 @@ struct nlmsghdr {
> __u16 nlmsg_flags;/* Additional flags */
> __u32 nlmsg_seq; /* Sequence number */
> __u32 nlmsg_pid; /* Sending process port ID */
> + __u8nlmsg_payload[];/* Contents of message */
> };
>
> /* Flags values */
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 1b5a9c2e1c29..09346aee1022 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2445,7 +2445,10 @@ void netlink_ack(struct sk_buff *in_skb, struct
> nlmsghdr *nlh, int err,
> NLMSG_ERROR, payload, flags);
> errmsg = nlmsg_data(rep);
> errmsg->error = err;
> - memcpy(>msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len :
> sizeof(*nlh));
> + errmsg->msg = *nlh;
> + if (payload > sizeof(*errmsg))
> + memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload,
> +nlh->nlmsg_len - sizeof(*nlh));
They have nlmsg_len()[1] for the length of the payload without the header:
/**
* nlmsg_len - length of message payload
* @nlh: netlink message header
*/
static inline int nlmsg_len(const struct nlmsghdr *nlh)
{
return nlh->nlmsg_len - NLMSG_HDRLEN;
}
(would that function use some sanitization, though? what if nlmsg_len is
somehow manipulated to be less than NLMSG_HDRLEN?...)
Also, it seems there is at least one more instance of this same issue:
diff --git a/net/netfilter/ipset/ip_set_core.c
b/net/netfilter/ipset/ip_set_core.c
index 16ae92054baa..d06184b94af5 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1723,7 +1723,8 @@ call_ad(struct net *net, struct sock *ctnl, struct
sk_buff *skb,
nlh->nlmsg_seq, NLMSG_ERROR, payload, 0);
errmsg = nlmsg_data(rep);
errmsg->error = ret;
- memcpy(>msg, nlh, nlh->nlmsg_len);
+ errmsg->msg = *nlh;
+ memcpy(errmsg->msg.nlmsg_payload, nlh->nlmsg_payload,
nlmsg_len(nlh));
cmdattr = (void *)>msg + min_len;
ret = nla_parse(cda, IPSET_ATTR_CMD_MAX, cmdattr,
--
Gustavo
[1] https://elixir.bootlin.com/linux/v5.18-rc5/source/include/net/netlink.h#L577
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, Dec 01, 2020 at 12:52:27AM -0500, Martin K. Petersen wrote: > > Gustavo, > > > This series aims to fix almost all remaining fall-through warnings in > > order to enable -Wimplicit-fallthrough for Clang. > > Applied 20-22,54,120-124 to 5.11/scsi-staging, thanks. Awesome! :) Thanks, Martin. -- Gustavo
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 08:38:46PM +, Mark Brown 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. > > > > [...] > > Applied to > >https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git > for-next > > Thanks! > > [1/1] regulator: as3722: Fix fall-through warnings for Clang > commit: b52b417ccac4fae5b1f2ec4f1d46eb91e4493dc5 Thank you, Mark. -- Gustavo
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 04:03:45PM -0400, Jason Gunthorpe wrote: > On Fri, Nov 20, 2020 at 12:21:39PM -0600, Gustavo A. R. Silva wrote: > > > IB/hfi1: Fix fall-through warnings for Clang > > IB/mlx4: Fix fall-through warnings for Clang > > IB/qedr: Fix fall-through warnings for Clang > > RDMA/mlx5: Fix fall-through warnings for Clang > > I picked these four to the rdma tree, thanks Awesome. :) Thank you, Jason. -- Gustavo
Re: [PATCH 058/141] xen-blkfront: Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 04:36:26PM -0500, boris.ostrov...@oracle.com wrote: > > On 11/20/20 1:32 PM, Gustavo A. R. Silva wrote: > > In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning > > by explicitly adding a break statement instead of letting the code fall > > through to the next case. > > > > Link: https://github.com/KSPP/linux/issues/115 > > Signed-off-by: Gustavo A. R. Silva > > --- > > drivers/block/xen-blkfront.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > > index 48629d3433b4..34b028be78ab 100644 > > --- a/drivers/block/xen-blkfront.c > > +++ b/drivers/block/xen-blkfront.c > > @@ -2462,6 +2462,7 @@ static void blkback_changed(struct xenbus_device *dev, > > break; > > if (talk_to_blkback(dev, info)) > > break; > > + break; > > case XenbusStateInitialising: > > case XenbusStateInitialised: > > case XenbusStateReconfiguring: > > > Reviewed-by Boris Ostrovsky > > > (for patch 138 as well) Thank you for both reviews, Boris. > Although I thought using 'fallthrough' attribute was the more common approach. I've got it. I will consider that for a future patch. Thanks -- Gustavo
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, Nov 22, 2020 at 11:53:55AM -0800, James Bottomley wrote: > On Sun, 2020-11-22 at 11:22 -0800, Joe Perches wrote: > > On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote: > > > 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 ... > > > > Gustavo? Are you running for congress now? > > > > https://lwn.net/Articles/794944/ > > That's 21 reported fixes of which about 50% seem to produce no change > in code behaviour at all, a quarter seem to have no user visible effect > with the remaining quarter producing unexpected errors on obscure > configuration parameters, which is why no-one really noticed them > before. The really important point here is the number of bugs this has prevented and will prevent in the future. See an example of this, below: https://lore.kernel.org/linux-iio/20190813135802.gb27...@kroah.com/ This work is still relevant, even if the total number of issues/bugs we find in the process is zero (which is not the case). "The sucky thing about doing hard work to deploy hardening is that the result is totally invisible by definition (things not happening) [..]" - Dmitry Vyukov Thanks -- Gustavo
[PATCH 000/141] Fix fall-through warnings for Clang
Hi all,
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].
Now that the -Wimplicit-fallthrough option has been globally enabled[5],
any compiler should really warn on missing either a fallthrough annotation
or any of the other case-terminating statements (break/continue/return/
goto) when falling through to the next case statement. Making exceptions
to this introduces variation in case handling which may continue to lead
to bugs, misunderstandings, and a general lack of robustness. The point
of enabling options like -Wimplicit-fallthrough is to prevent human error
and aid developers in spotting bugs before their code is even built/
submitted/committed, therefore eliminating classes of bugs. So, in order
to really accomplish this, we should, and can, move in the direction of
addressing any error-prone scenarios and get rid of the unintentional
fallthrough bug-class in the kernel, entirely, even if there is some minor
redundancy. Better to have explicit case-ending statements than continue to
have exceptions where one must guess as to the right result. The compiler
will eliminate any actual redundancy.
Note that there is already a patch in mainline that addresses almost
40,000 of these issues[6].
I'm happy to carry this whole series in my own tree if people are OK
with it. :)
[1] commit e2079e93f562c ("kbuild: Do not enable -Wimplicit-fallthrough for
clang for now")
[2] ClangBuiltLinux#636
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
[4] https://godbolt.org/z/xgkvIh
[5] commit a035d552a93b ("Makefile: Globally enable fall-through warning")
[6] commit 4169e889e588 ("include: jhash/signal: Fix fall-through warnings for
Clang")
Thanks!
Gustavo A. R. Silva (141):
afs: Fix fall-through warnings for Clang
ASoC: codecs: Fix fall-through warnings for Clang
cifs: Fix fall-through warnings for Clang
drm/amdgpu: Fix fall-through warnings for Clang
drm/radeon: Fix fall-through warnings for Clang
gfs2: Fix fall-through warnings for Clang
gpio: Fix fall-through warnings for Clang
IB/hfi1: Fix fall-through warnings for Clang
igb: Fix fall-through warnings for Clang
ima: Fix fall-through warnings for Clang
ipv4: Fix fall-through warnings for Clang
ixgbe: Fix fall-through warnings for Clang
media: dvb-frontends: Fix fall-through warnings for Clang
media: usb: dvb-usb-v2: Fix fall-through warnings for Clang
netfilter: Fix fall-through warnings for Clang
nfsd: Fix fall-through warnings for Clang
nfs: Fix fall-through warnings for Clang
qed: Fix fall-through warnings for Clang
qlcnic: Fix fall-through warnings for Clang
scsi: aic7xxx: Fix fall-through warnings for Clang
scsi: aic94xx: Fix fall-through warnings for Clang
scsi: bfa: Fix fall-through warnings for Clang
staging: rtl8723bs: core: Fix fall-through warnings for Clang
staging: vt6655: Fix fall-through warnings for Clang
bnxt_en: Fix fall-through warnings for Clang
ceph: Fix fall-through warnings for Clang
drbd: Fix fall-through warnings for Clang
drm/amd/display: Fix fall-through warnings for Clang
e1000: Fix fall-through warnings for Clang
ext2: Fix fall-through warnings for Clang
ext4: Fix fall-through warnings for Clang
floppy: Fix fall-through warnings for Clang
fm10k: Fix fall-through warnings for Clang
IB/mlx4: Fix fall-through warnings for Clang
IB/qedr: Fix fall-through warnings for Clang
ice: Fix fall-through warnings for Clang
Input: pcspkr - Fix fall-through warnings for Clang
isofs: Fix fall-through warnings for Clang
ixgbevf: Fix fall-through warnings for Clang
kprobes/x86: Fix fall-through warnings for Clang
mm: Fix fall-through warnings for Clang
net: 3c509: Fix fall-through warnings for Clang
net: cassini: Fix fall-through warnings for Clang
net/mlx4: Fix fall-through warnings for Clang
net: mscc: ocelot: Fix fall-through warnings for Clang
netxen_nic: Fix fall-through warnings for Clang
nfp: Fix fall-through warnings for Clang
perf/x86: Fix fall-through warnings for Clang
pinctrl: Fix fall-through warnings for Clang
RDMA/mlx5: Fix fall-through warnings for Clang
reiserfs: Fix fall-through warnings for Clang
security: keys: Fix fall-through warnings for Clang
selinux: Fix fall-through warnings for Clang
target: Fix fall-through warnings for Clang
[PATCH 058/141] xen-blkfront: Fix fall-through warnings for Clang
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement instead of letting the code fall through to the next case. Link: https://github.com/KSPP/linux/issues/115 Signed-off-by: Gustavo A. R. Silva --- drivers/block/xen-blkfront.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index 48629d3433b4..34b028be78ab 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -2462,6 +2462,7 @@ static void blkback_changed(struct xenbus_device *dev, break; if (talk_to_blkback(dev, info)) break; + break; case XenbusStateInitialising: case XenbusStateInitialised: case XenbusStateReconfiguring: -- 2.27.0
[PATCH 138/141] xen/manage: Fix fall-through warnings for Clang
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement instead of letting the code fall through to the next case. Link: https://github.com/KSPP/linux/issues/115 Signed-off-by: Gustavo A. R. Silva --- drivers/xen/manage.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c index cd046684e0d1..374d36de7f5a 100644 --- a/drivers/xen/manage.c +++ b/drivers/xen/manage.c @@ -179,6 +179,7 @@ static int poweroff_nb(struct notifier_block *cb, unsigned long code, void *unus case SYS_HALT: case SYS_POWER_OFF: shutting_down = SHUTDOWN_POWEROFF; + break; default: break; } -- 2.27.0
Re: [PATCH 000/141] Fix fall-through warnings for Clang
Hi,
On 11/20/20 12:53, 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?!
The justification for this is explained in this same changelog text:
Now that the -Wimplicit-fallthrough option has been globally enabled[5],
any compiler should really warn on missing either a fallthrough annotation
or any of the other case-terminating statements (break/continue/return/
goto) when falling through to the next case statement. Making exceptions
to this introduces variation in case handling which may continue to lead
to bugs, misunderstandings, and a general lack of robustness. The point
of enabling options like -Wimplicit-fallthrough is to prevent human error
and aid developers in spotting bugs before their code is even built/
submitted/committed, therefore eliminating classes of bugs. So, in order
to really accomplish this, we should, and can, move in the direction of
addressing any error-prone scenarios and get rid of the unintentional
fallthrough bug-class in the kernel, entirely, even if there is some minor
redundancy. Better to have explicit case-ending statements than continue to
have exceptions where one must guess as to the right result. The compiler
will eliminate any actual redundancy.
Note that there is already a patch in mainline that addresses almost
40,000 of these issues[6].
[1] commit e2079e93f562c ("kbuild: Do not enable -Wimplicit-fallthrough for
clang for now")
[2] ClangBuiltLinux#636
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
[4] https://godbolt.org/z/xgkvIh
[5] commit a035d552a93b ("Makefile: Globally enable fall-through warning")
[6] commit 4169e889e588 ("include: jhash/signal: Fix fall-through warnings for
Clang")
Thanks
--
Gustavo
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On 11/20/20 12:28, Joe Perches wrote: > On Fri, 2020-11-20 at 12:21 -0600, Gustavo A. R. Silva wrote: >> Hi all, >> >> 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. > > This was a bit hard to parse for a second or three. > > Thanks Gustavo. > > How was this change done? I audited case by case in order to determine the best fit for each situation. Depending on the surrounding logic, sometimes it makes more sense a goto or a fallthrough rather than merely a break. Thanks -- Gustavo
[Xen-devel] [PATCH] xen: Replace zero-length array with flexible-array member
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.
Also, notice that, dynamic memory allocations won't be affected by
this change:
"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
drivers/xen/xen-pciback/pciback.h | 2 +-
include/xen/interface/io/tpmif.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/xen-pciback/pciback.h
b/drivers/xen/xen-pciback/pciback.h
index ce1077e32466..7c95516a860f 100644
--- a/drivers/xen/xen-pciback/pciback.h
+++ b/drivers/xen/xen-pciback/pciback.h
@@ -52,7 +52,7 @@ struct xen_pcibk_dev_data {
unsigned int ack_intr:1; /* .. and ACK-ing */
unsigned long handled;
unsigned int irq; /* Saved in case device transitions to MSI/MSI-X */
- char irq_name[0]; /* xen-pcibk[000:04:00.0] */
+ char irq_name[]; /* xen-pcibk[000:04:00.0] */
};
/* Used by XenBus and xen_pcibk_ops.c */
diff --git a/include/xen/interface/io/tpmif.h b/include/xen/interface/io/tpmif.h
index 28e7dcd75e82..f8aa8bac5196 100644
--- a/include/xen/interface/io/tpmif.h
+++ b/include/xen/interface/io/tpmif.h
@@ -46,7 +46,7 @@ struct vtpm_shared_page {
uint8_t pad;
uint8_t nr_extra_pages; /* extra pages for long packets; may be zero */
- uint32_t extra_pages[0]; /* grant IDs; length in nr_extra_pages */
+ uint32_t extra_pages[]; /* grant IDs; length in nr_extra_pages */
};
#endif
--
2.25.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH net-next] xen-netfront: mark expected switch fall-through
On 4/16/19 2:17 AM, Juergen Gross wrote: > On 15/04/2019 23:11, Gustavo A. R. Silva wrote: >> In preparation to enabling -Wimplicit-fallthrough, mark switch >> cases where we are expecting to fall through. >> >> This patch fixes the following warning: >> >> drivers/net/xen-netfront.c: In function ‘netback_changed’: >> drivers/net/xen-netfront.c:2038:6: warning: this statement may fall through >> [-Wimplicit-fallthrough=] >>if (dev->state == XenbusStateClosed) >> ^ >> drivers/net/xen-netfront.c:2041:2: note: here >> case XenbusStateClosing: >> ^~~~ >> >> Warning level 3 was used: -Wimplicit-fallthrough=3 >> >> Notice that, in this particular case, the code comment is modified >> in accordance with what GCC is expecting to find. >> >> This patch is part of the ongoing efforts to enable >> -Wimplicit-fallthrough. >> >> Signed-off-by: Gustavo A. R. Silva > > Reviewed-by: Juergen Gross > Thanks, Juergen. -- Gustavo ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH net-next] xen-netfront: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. This patch fixes the following warning: drivers/net/xen-netfront.c: In function ‘netback_changed’: drivers/net/xen-netfront.c:2038:6: warning: this statement may fall through [-Wimplicit-fallthrough=] if (dev->state == XenbusStateClosed) ^ drivers/net/xen-netfront.c:2041:2: note: here case XenbusStateClosing: ^~~~ Warning level 3 was used: -Wimplicit-fallthrough=3 Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find. This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough. Signed-off-by: Gustavo A. R. Silva --- drivers/net/xen-netfront.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 80c30321de41..8d33970a2950 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -2037,7 +2037,7 @@ static void netback_changed(struct xenbus_device *dev, case XenbusStateClosed: if (dev->state == XenbusStateClosed) break; - /* Missed the backend's CLOSING state -- fallthrough */ + /* Fall through - Missed the backend's CLOSING state. */ case XenbusStateClosing: xenbus_frontend_closed(dev); break; -- 2.21.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] PCI: Mark expected switch fall-throughs
On 3/20/19 3:13 PM, Bjorn Helgaas wrote: > On Wed, Mar 20, 2019 at 01:27:15PM -0500, Gustavo A. R. Silva wrote: [..] >> >> Warning level 3 was used: -Wimplicit-fallthrough=3 >> >> This patch is part of the ongoing efforts to enable >> -Wimplicit-fallthrough. >> >> Signed-off-by: Gustavo A. R. Silva > > Applied to pci/misc for v5.2, thanks! > Awesome! Thanks -- Gustavo ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] PCI: Mark expected switch fall-throughs
On 3/20/19 2:27 PM, Andrew Cooper wrote: > On 20/03/2019 18:27, Gustavo A. R. Silva wrote: >> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c >> index 6fa1627ce08d..445b51db75b0 100644 >> --- a/drivers/pci/proc.c >> +++ b/drivers/pci/proc.c >> @@ -222,6 +222,7 @@ static long proc_bus_pci_ioctl(struct file *file, >> unsigned int cmd, >> } >> /* If arch decided it can't, fall through... */ >> #endif /* HAVE_PCI_MMAP */ >> +/* fall through */ > > Surely it would be better to transpose the #endif and its previous line, > than to add a second fallthrough ? > I agree. The thing is that, currently, GCC is expecting to find the fall-through "annotations" at the very bottom of the case statement, as I mentioned it in the changelog text. That's the reason why I decided to left in place the original comment. Thanks -- Gustavo ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] PCI: Mark expected switch fall-throughs
On 3/20/19 2:24 PM, Bjorn Helgaas wrote:
> On Wed, Mar 20, 2019 at 01:27:15PM -0500, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wimplicit-fallthrough, mark switch
>> cases where we are expecting to fall through.
>
> Does this fix all the remaining cases in drivers/pci? I'd like to fix
> them all at once.
>
These are actually the last ones.
> What's the best way to watch for new warnings being added? I fiddled
> with "make W=2" etc but it didn't seem useful. Does the 0-day robot
> warn when -Wimplicit-fallthrough warnings are added?
>
0-day robot doesn't know about these warnings yet. But it certainly
will once we finally enable -Wimplicit-fallthrough.
Add this to your Makefile to see the warnings:
KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough=3,)
Thanks
--
Gustavo
> Bjorn
>
>> This patch fixes the following warnings:
>>
>> drivers/pci/proc.c: In function ‘proc_bus_pci_ioctl’:
>> drivers/pci/proc.c:216:6: warning: this statement may fall through
>> [-Wimplicit-fallthrough=]
>>if (arch_can_pci_mmap_wc()) {
>> ^
>> drivers/pci/proc.c:225:2: note: here
>> default:
>> ^~~
>>
>> drivers/pci/xen-pcifront.c: In function ‘pcifront_backend_changed’:
>> drivers/pci/xen-pcifront.c:1105:6: warning: this statement may fall through
>> [-Wimplicit-fallthrough=]
>>if (xdev->state == XenbusStateClosed)
>> ^
>> drivers/pci/xen-pcifront.c:1108:2: note: here
>> case XenbusStateClosing:
>> ^~~~
>>
>> Notice that, in this particular case, the /* fall through */
>> comment is placed at the very bottom of the case statement,
>> which is what GCC is expecting to find.
>>
>> Warning level 3 was used: -Wimplicit-fallthrough=3
>>
>> This patch is part of the ongoing efforts to enable
>> -Wimplicit-fallthrough.
>>
>> Signed-off-by: Gustavo A. R. Silva
>> ---
>> drivers/pci/proc.c | 1 +
>> drivers/pci/xen-pcifront.c | 2 +-
>> 2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
>> index 6fa1627ce08d..445b51db75b0 100644
>> --- a/drivers/pci/proc.c
>> +++ b/drivers/pci/proc.c
>> @@ -222,6 +222,7 @@ static long proc_bus_pci_ioctl(struct file *file,
>> unsigned int cmd,
>> }
>> /* If arch decided it can't, fall through... */
>> #endif /* HAVE_PCI_MMAP */
>> +/* fall through */
>> default:
>> ret = -EINVAL;
>> break;
>> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
>> index eba6e33147a2..14cf0f41ecf0 100644
>> --- a/drivers/pci/xen-pcifront.c
>> +++ b/drivers/pci/xen-pcifront.c
>> @@ -1104,7 +1104,7 @@ static void __ref pcifront_backend_changed(struct
>> xenbus_device *xdev,
>> case XenbusStateClosed:
>> if (xdev->state == XenbusStateClosed)
>> break;
>> -/* Missed the backend's CLOSING state -- fallthrough */
>> +/* fall through - Missed the backend's CLOSING state. */
>> case XenbusStateClosing:
>> dev_warn(>dev, "backend going away!\n");
>> pcifront_try_disconnect(pdev);
>> --
>> 2.21.0
>>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] PCI: Mark expected switch fall-throughs
In preparation to enabling -Wimplicit-fallthrough, mark switch
cases where we are expecting to fall through.
This patch fixes the following warnings:
drivers/pci/proc.c: In function ‘proc_bus_pci_ioctl’:
drivers/pci/proc.c:216:6: warning: this statement may fall through
[-Wimplicit-fallthrough=]
if (arch_can_pci_mmap_wc()) {
^
drivers/pci/proc.c:225:2: note: here
default:
^~~
drivers/pci/xen-pcifront.c: In function ‘pcifront_backend_changed’:
drivers/pci/xen-pcifront.c:1105:6: warning: this statement may fall through
[-Wimplicit-fallthrough=]
if (xdev->state == XenbusStateClosed)
^
drivers/pci/xen-pcifront.c:1108:2: note: here
case XenbusStateClosing:
^~~~
Notice that, in this particular case, the /* fall through */
comment is placed at the very bottom of the case statement,
which is what GCC is expecting to find.
Warning level 3 was used: -Wimplicit-fallthrough=3
This patch is part of the ongoing efforts to enable
-Wimplicit-fallthrough.
Signed-off-by: Gustavo A. R. Silva
---
drivers/pci/proc.c | 1 +
drivers/pci/xen-pcifront.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index 6fa1627ce08d..445b51db75b0 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -222,6 +222,7 @@ static long proc_bus_pci_ioctl(struct file *file, unsigned
int cmd,
}
/* If arch decided it can't, fall through... */
#endif /* HAVE_PCI_MMAP */
+ /* fall through */
default:
ret = -EINVAL;
break;
diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index eba6e33147a2..14cf0f41ecf0 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -1104,7 +1104,7 @@ static void __ref pcifront_backend_changed(struct
xenbus_device *xdev,
case XenbusStateClosed:
if (xdev->state == XenbusStateClosed)
break;
- /* Missed the backend's CLOSING state -- fallthrough */
+ /* fall through - Missed the backend's CLOSING state. */
case XenbusStateClosing:
dev_warn(>dev, "backend going away!\n");
pcifront_try_disconnect(pdev);
--
2.21.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: mark expected switch fall-through
On 2/12/19 5:50 PM, Boris Ostrovsky wrote: > On Tue, Feb 12, 2019 at 02:37:20PM -0600, Gustavo A. R. Silva wrote: >> In preparation to enabling -Wimplicit-fallthrough, mark switch >> cases where we are expecting to fall through. >> >> This patch fixes the following warning: >> >> drivers/xen/xen-pciback/xenbus.c: In function ‘xen_pcibk_frontend_changed’: >> drivers/xen/xen-pciback/xenbus.c:545:6: warning: this statement may fall >> through [-Wimplicit-fallthrough=] >>if (xenbus_dev_is_online(xdev)) >> ^ >> drivers/xen/xen-pciback/xenbus.c:548:2: note: here >> case XenbusStateUnknown: >> ^~~~ >> >> Warning level 3 was used: -Wimplicit-fallthrough=3 >> >> Notice that, in this particular case, the code comment is modified >> in accordance with what GCC is expecting to find. >> >> This patch is part of the ongoing efforts to enable >> -Wimplicit-fallthrough. >> >> Signed-off-by: Gustavo A. R. Silva > > Applied to for-linus-5.0 > > (xen-scsiback patch too) > Thank you, Boris. -- Gustavo ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] xen-scsiback: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. This patch fixes the following warning: drivers/xen/xen-scsiback.c: In function ‘scsiback_frontend_changed’: drivers/xen/xen-scsiback.c:1185:6: warning: this statement may fall through [-Wimplicit-fallthrough=] if (xenbus_dev_is_online(dev)) ^ drivers/xen/xen-scsiback.c:1188:2: note: here case XenbusStateUnknown: ^~~~ Warning level 3 was used: -Wimplicit-fallthrough=3 Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find. This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough. Signed-off-by: Gustavo A. R. Silva --- drivers/xen/xen-scsiback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c index e59937293a32..ba0942e481bc 100644 --- a/drivers/xen/xen-scsiback.c +++ b/drivers/xen/xen-scsiback.c @@ -1184,7 +1184,7 @@ static void scsiback_frontend_changed(struct xenbus_device *dev, xenbus_switch_state(dev, XenbusStateClosed); if (xenbus_dev_is_online(dev)) break; - /* fall through if not online */ + /* fall through - if not online */ case XenbusStateUnknown: device_unregister(>dev); break; -- 2.20.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] xen: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. This patch fixes the following warning: drivers/xen/xen-pciback/xenbus.c: In function ‘xen_pcibk_frontend_changed’: drivers/xen/xen-pciback/xenbus.c:545:6: warning: this statement may fall through [-Wimplicit-fallthrough=] if (xenbus_dev_is_online(xdev)) ^ drivers/xen/xen-pciback/xenbus.c:548:2: note: here case XenbusStateUnknown: ^~~~ Warning level 3 was used: -Wimplicit-fallthrough=3 Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find. This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough. Signed-off-by: Gustavo A. R. Silva --- drivers/xen/xen-pciback/xenbus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c index 581c4e1a8b82..23f7f6ec7d1f 100644 --- a/drivers/xen/xen-pciback/xenbus.c +++ b/drivers/xen/xen-pciback/xenbus.c @@ -544,7 +544,7 @@ static void xen_pcibk_frontend_changed(struct xenbus_device *xdev, xenbus_switch_state(xdev, XenbusStateClosed); if (xenbus_dev_is_online(xdev)) break; - /* fall through if not online */ + /* fall through - if not online */ case XenbusStateUnknown: dev_dbg(>dev, "frontend is gone! unregister device\n"); device_unregister(>dev); -- 2.20.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH net-next] xen-netback: mark expected switch fall-through
On 2/8/19 2:21 PM, David Miller wrote: > From: "Gustavo A. R. Silva" > Date: Fri, 8 Feb 2019 13:58:38 -0600 > >> In preparation to enabling -Wimplicit-fallthrough, mark switch >> cases where we are expecting to fall through. >> >> Warning level 3 was used: -Wimplicit-fallthrough=3 >> >> This patch is part of the ongoing efforts to enabling >> -Wimplicit-fallthrough. >> >> Signed-off-by: Gustavo A. R. Silva > > Applied. > Thanks, Dave. -- Gustavo ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH net-next] xen-netback: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Warning level 3 was used: -Wimplicit-fallthrough=3 This patch is part of the ongoing efforts to enabling -Wimplicit-fallthrough. Signed-off-by: Gustavo A. R. Silva --- drivers/net/xen-netback/xenbus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c index 2625740bdc4a..330ddb64930f 100644 --- a/drivers/net/xen-netback/xenbus.c +++ b/drivers/net/xen-netback/xenbus.c @@ -655,7 +655,7 @@ static void frontend_changed(struct xenbus_device *dev, set_backend_state(be, XenbusStateClosed); if (xenbus_dev_is_online(dev)) break; - /* fall through if not online */ + /* fall through - if not online */ case XenbusStateUnknown: set_backend_state(be, XenbusStateClosed); device_unregister(>dev); -- 2.20.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 00/41] scsi: Mark expected switch fall-throughs
On 12/18/18 9:45 PM, Martin K. Petersen wrote: If you haven't received feedback on a patch you should poke the relevant driver maintainer. Got it. Will do so. Thanks -- Gustavo ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 00/41] scsi: Mark expected switch fall-throughs
Hi Martin, Friendly ping: Only 8 out the 41 patches in this series have been applied so far. I wonder if you could apply the rest of this series, except: [PATCH 02/41] scsi: NCR5380: Mark expected switch fall-through (I'll send a v2 of this patch) Thanks -- Gustavo On 11/27/18 10:18 PM, Gustavo A. R. Silva wrote: In preparation to enabling -Wimplicit-fallthrough, this patchset aims to mark switch cases where we are expecting to fall through. I reviewed case by case and concluded that each of them is an intentional fall-through. However, it doesn't hurt that the maintainers and supporters of each driver take a look. :) Each commit log contains the particular details for the changes in the corresponding file. This series fix a total of 110 of the following type of warnings in drivers/scsi: drivers/scsi/aic7xxx/aic7xxx_core.c:4921:3: warning: this statement may fall through [-Wimplicit-fallthrough=] ahc_dma_tag_destroy(ahc, scb_data->sg_dmat); ^~~ drivers/scsi/aic7xxx/aic7xxx_core.c:4923:2: note: here case 6: ^~~~ Thanks! Gustavo A. R. Silva (41): scsi: BusLogic: mark expected switch fall-through scsi: NCR5380: Mark expected switch fall-through scsi: aacraid: aachba: Mark expected switch fall-throughs scsi: aacraid: linit: Mark expected switch fall-through scsi: aic7xxx: aic79xx: mark expected switch fall-through scsi: aic7xxx: mark expected switch fall-throughs scsi: be2iscsi: be_iscsi: Mark expected switch fall-through scsi: be2iscsi: be_main: Mark expected switch fall-through scsi: bfa: bfa_fcpim: Mark expected switch fall-throughs scsi: bfa: bfa_fcs_lport: Mark expected switch fall-throughs scsi: bfa: bfa_fcs_rport: Mark expected switch fall-throughs scsi: bfa: bfa_ioc: Mark expected switch fall-throughs scsi: csiostor: csio_wr: mark expected switch fall-through scsi: esas2r: esas2r_init: mark expected switch fall-throughs scsi: hpsa: mark expected switch fall-throughs scsi: imm: mark expected switch fall-throughs scsi: isci: phy: Mark expected switch fall-through scsi: isci: remote_device: Mark expected switch fall-throughs scsi: isci: remote_node_context: mark expected switch fall-throughs scsi: isci: request: mark expected switch fall-through scsi: libfc: fc_rport: Mark expected switch fall-through scsi: lpfc: lpfc_ct: Mark expected switch fall-throughs scsi: lpfc: lpfc_els: Mark expected switch fall-throughs scsi: lpfc: lpfc_hbadisc: Mark expected switch fall-throughs scsi: lpfc: lpfc_nportdisc: Mark expected switch fall-through scsi: lpfc: lpfc_nvme: Mark expected switch fall-through scsi: lpfc: lpfc_scsi: Mark expected switch fall-throughs scsi: lpfc: lpfc_sli: Mark expected switch fall-throughs scsi: megaraid: megaraid_sas_base: Mark expected switch fall-through scsi: megaraid_sas_fusion: Mark expected switch fall-through scsi: mpt3sas: mpt3sas_scsih: Mark expected switch fall-through scsi: myrb: Mark expected switch fall-throughs scsi: osd: osd_initiator: mark expected switch fall-throughs scsi: osst: mark expected switch fall-throughs scsi: ppa: mark expected switch fall-through scsi: qla4xxx: ql4_os: mark expected switch fall-through scsi: st: mark expected switch fall-throughs scsi: sym53c8xx_2: sym_hipd: mark expected switch fall-throughs scsi: sym53c8xx_2: sym_nvram: Mark expected switch fall-through scsi: ufs: ufshcd: mark expected switch fall-throughs scsi: xen-scsifront: mark expected switch fall-through drivers/scsi/BusLogic.c | 1 + drivers/scsi/NCR5380.c | 3 +- drivers/scsi/aacraid/aachba.c | 5 +++- drivers/scsi/aacraid/linit.c| 1 + drivers/scsi/aic7xxx/aic79xx_core.c | 14 + drivers/scsi/aic7xxx/aic7xxx_core.c | 12 ++-- drivers/scsi/be2iscsi/be_iscsi.c| 1 + drivers/scsi/be2iscsi/be_main.c | 1 + drivers/scsi/bfa/bfa_fcpim.c| 6 ++-- drivers/scsi/bfa/bfa_fcs_lport.c| 8 ++--- drivers/scsi/bfa/bfa_fcs_rport.c| 19 +--- drivers/scsi/bfa/bfa_ioc.c | 9 ++ drivers/scsi/csiostor/csio_wr.c | 1 + drivers/scsi/esas2r/esas2r_init.c | 3 +- drivers/scsi/hpsa.c | 5 drivers/scsi/imm.c | 33 +++-- drivers/scsi/isci/phy.c | 1 + drivers/scsi/isci/remote_device.c | 4 +-- drivers/scsi/isci/remote_node_context.c | 4 +-- drivers/scsi/isci/request.c | 2 +- drivers/scsi/libfc/fc_rport.c | 1 + drivers/scsi/lpfc/lpfc_ct.c | 2 ++ drivers/scsi/lpfc/lpfc_els.c| 1 + drivers/scsi/lpfc/lpfc_hbadisc.c| 4 ++- drivers/scsi/lpfc/lpfc_nportdisc.c | 1 + drivers/scsi/l
[Xen-devel] [PATCH 41/41] scsi: xen-scsifront: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Notice that, in this particular case, I replaced "Missed the backend's Closing state -- fallthrough" with "fall through - Missed the backend's Closing state", which contains the "fall through" annotation at the beginnig of the code comment, which is what GCC is expecting to find. Signed-off-by: Gustavo A. R. Silva --- drivers/scsi/xen-scsifront.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c index 61389bdc7926..bb76d0d2022b 100644 --- a/drivers/scsi/xen-scsifront.c +++ b/drivers/scsi/xen-scsifront.c @@ -1112,7 +1112,7 @@ static void scsifront_backend_changed(struct xenbus_device *dev, case XenbusStateClosed: if (dev->state == XenbusStateClosed) break; - /* Missed the backend's Closing state -- fallthrough */ + /* fall through - Missed the backend's Closing state */ case XenbusStateClosing: scsifront_disconnect(info); break; -- 2.17.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 00/41] scsi: Mark expected switch fall-throughs
In preparation to enabling -Wimplicit-fallthrough, this patchset aims to mark switch cases where we are expecting to fall through. I reviewed case by case and concluded that each of them is an intentional fall-through. However, it doesn't hurt that the maintainers and supporters of each driver take a look. :) Each commit log contains the particular details for the changes in the corresponding file. This series fix a total of 110 of the following type of warnings in drivers/scsi: drivers/scsi/aic7xxx/aic7xxx_core.c:4921:3: warning: this statement may fall through [-Wimplicit-fallthrough=] ahc_dma_tag_destroy(ahc, scb_data->sg_dmat); ^~~ drivers/scsi/aic7xxx/aic7xxx_core.c:4923:2: note: here case 6: ^~~~ Thanks! Gustavo A. R. Silva (41): scsi: BusLogic: mark expected switch fall-through scsi: NCR5380: Mark expected switch fall-through scsi: aacraid: aachba: Mark expected switch fall-throughs scsi: aacraid: linit: Mark expected switch fall-through scsi: aic7xxx: aic79xx: mark expected switch fall-through scsi: aic7xxx: mark expected switch fall-throughs scsi: be2iscsi: be_iscsi: Mark expected switch fall-through scsi: be2iscsi: be_main: Mark expected switch fall-through scsi: bfa: bfa_fcpim: Mark expected switch fall-throughs scsi: bfa: bfa_fcs_lport: Mark expected switch fall-throughs scsi: bfa: bfa_fcs_rport: Mark expected switch fall-throughs scsi: bfa: bfa_ioc: Mark expected switch fall-throughs scsi: csiostor: csio_wr: mark expected switch fall-through scsi: esas2r: esas2r_init: mark expected switch fall-throughs scsi: hpsa: mark expected switch fall-throughs scsi: imm: mark expected switch fall-throughs scsi: isci: phy: Mark expected switch fall-through scsi: isci: remote_device: Mark expected switch fall-throughs scsi: isci: remote_node_context: mark expected switch fall-throughs scsi: isci: request: mark expected switch fall-through scsi: libfc: fc_rport: Mark expected switch fall-through scsi: lpfc: lpfc_ct: Mark expected switch fall-throughs scsi: lpfc: lpfc_els: Mark expected switch fall-throughs scsi: lpfc: lpfc_hbadisc: Mark expected switch fall-throughs scsi: lpfc: lpfc_nportdisc: Mark expected switch fall-through scsi: lpfc: lpfc_nvme: Mark expected switch fall-through scsi: lpfc: lpfc_scsi: Mark expected switch fall-throughs scsi: lpfc: lpfc_sli: Mark expected switch fall-throughs scsi: megaraid: megaraid_sas_base: Mark expected switch fall-through scsi: megaraid_sas_fusion: Mark expected switch fall-through scsi: mpt3sas: mpt3sas_scsih: Mark expected switch fall-through scsi: myrb: Mark expected switch fall-throughs scsi: osd: osd_initiator: mark expected switch fall-throughs scsi: osst: mark expected switch fall-throughs scsi: ppa: mark expected switch fall-through scsi: qla4xxx: ql4_os: mark expected switch fall-through scsi: st: mark expected switch fall-throughs scsi: sym53c8xx_2: sym_hipd: mark expected switch fall-throughs scsi: sym53c8xx_2: sym_nvram: Mark expected switch fall-through scsi: ufs: ufshcd: mark expected switch fall-throughs scsi: xen-scsifront: mark expected switch fall-through drivers/scsi/BusLogic.c | 1 + drivers/scsi/NCR5380.c | 3 +- drivers/scsi/aacraid/aachba.c | 5 +++- drivers/scsi/aacraid/linit.c| 1 + drivers/scsi/aic7xxx/aic79xx_core.c | 14 + drivers/scsi/aic7xxx/aic7xxx_core.c | 12 ++-- drivers/scsi/be2iscsi/be_iscsi.c| 1 + drivers/scsi/be2iscsi/be_main.c | 1 + drivers/scsi/bfa/bfa_fcpim.c| 6 ++-- drivers/scsi/bfa/bfa_fcs_lport.c| 8 ++--- drivers/scsi/bfa/bfa_fcs_rport.c| 19 +--- drivers/scsi/bfa/bfa_ioc.c | 9 ++ drivers/scsi/csiostor/csio_wr.c | 1 + drivers/scsi/esas2r/esas2r_init.c | 3 +- drivers/scsi/hpsa.c | 5 drivers/scsi/imm.c | 33 +++-- drivers/scsi/isci/phy.c | 1 + drivers/scsi/isci/remote_device.c | 4 +-- drivers/scsi/isci/remote_node_context.c | 4 +-- drivers/scsi/isci/request.c | 2 +- drivers/scsi/libfc/fc_rport.c | 1 + drivers/scsi/lpfc/lpfc_ct.c | 2 ++ drivers/scsi/lpfc/lpfc_els.c| 1 + drivers/scsi/lpfc/lpfc_hbadisc.c| 4 ++- drivers/scsi/lpfc/lpfc_nportdisc.c | 1 + drivers/scsi/lpfc/lpfc_nvme.c | 1 + drivers/scsi/lpfc/lpfc_scsi.c | 8 ++--- drivers/scsi/lpfc/lpfc_sli.c| 20 +++-- drivers/scsi/megaraid/megaraid_sas_base.c | 1 + drivers/scsi/megaraid/megaraid_sas_fusion.c | 2 +- drivers/scsi/mpt3sas/mpt3sas_scsih.c| 1 + drivers/scsi/myrb.c | 3 ++ drivers/scsi/osd/osd_initiato
Re: [Xen-devel] [PATCH] xen/biomerge: Use true and false for boolean values
On 08/06/2018 06:42 AM, Juergen Gross wrote: > On 05/08/18 02:50, Gustavo A. R. Silva wrote: >> Return statements in functions returning bool should use true or false >> instead of an integer value. >> >> This code was detected with the help of Coccinelle. >> >> Signed-off-by: Gustavo A. R. Silva > > Reviewed-by: Juergen Gross > Thanks, Juergen. -- Gustavo ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] xen/biomerge: Use true and false for boolean values
Return statements in functions returning bool should use true or false instead of an integer value. This code was detected with the help of Coccinelle. Signed-off-by: Gustavo A. R. Silva --- drivers/xen/biomerge.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/xen/biomerge.c b/drivers/xen/biomerge.c index 30d7f52..55ed80c 100644 --- a/drivers/xen/biomerge.c +++ b/drivers/xen/biomerge.c @@ -17,7 +17,7 @@ bool xen_biovec_phys_mergeable(const struct bio_vec *vec1, * XXX: Add support for merging bio_vec when using different page * size in Xen and Linux. */ - return 0; + return false; #endif } EXPORT_SYMBOL(xen_biovec_phys_mergeable); -- 2.7.4 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] xen-blkfront: use true and false for boolean values
Return statements in functions returning bool should use true or false instead of an integer value. This code was detected with the help of Coccinelle. Signed-off-by: Gustavo A. R. Silva --- drivers/block/xen-blkfront.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index 94300db..8986ada 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -1436,7 +1436,7 @@ static bool blkif_completion(unsigned long *id, /* Wait the second response if not yet here. */ if (s2->status == REQ_WAITING) - return 0; + return false; bret->status = blkif_get_final_status(s->status, s2->status); @@ -1537,7 +1537,7 @@ static bool blkif_completion(unsigned long *id, } } - return 1; + return true; } static irqreturn_t blkif_interrupt(int irq, void *dev_id) -- 2.7.4 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH net-next] xen-netback: use true and false for boolean values
Return statements in functions returning bool should use true or false
instead of an integer value.
This issue was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva
---
drivers/net/xen-netback/netback.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/xen-netback/netback.c
b/drivers/net/xen-netback/netback.c
index a27daa2..3621e05 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1603,9 +1603,9 @@ static void xenvif_ctrl_action(struct xenvif *vif)
static bool xenvif_ctrl_work_todo(struct xenvif *vif)
{
if (likely(RING_HAS_UNCONSUMED_REQUESTS(>ctrl)))
- return 1;
+ return true;
- return 0;
+ return false;
}
irqreturn_t xenvif_ctrl_irq_fn(int irq, void *data)
--
2.7.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
