Hi all,
After merging the vfs-brauner tree, today's linux-next build (arm
multi_v7_defconfig) produced this warning:
fs/coredump.c: In function 'dump_user_range':
fs/coredump.c:923:40: warning: left-hand operand of comma expression has no effect [-Wunused-value]
923 | #define dump_page_copy(src, dst) ((dst), (src))
| ^
fs/coredump.c:948:58: note: in expansion of macro 'dump_page_copy'
948 | int stop = !dump_emit_page(cprm, dump_page_copy(page, dump_page));
| ^~~~~~~~~~~~~~
Introduced by commit
4630f2caafcd ("coredump: get machine check errors early rather than during iov_iter")
--
Cheers,
Stephen Rothwell
On Tue, 5 Mar 2024 at 15:51, Stephen Rothwell <[email protected]> wrote:
>
> fs/coredump.c: In function 'dump_user_range':
> fs/coredump.c:923:40: warning: left-hand operand of comma expression has no effect [-Wunused-value]
> 923 | #define dump_page_copy(src, dst) ((dst), (src))
> | ^
> fs/coredump.c:948:58: note: in expansion of macro 'dump_page_copy'
> 948 | int stop = !dump_emit_page(cprm, dump_page_copy(page, dump_page));
> | ^~~~~~~~~~~~~~
>
> Introduced by commit
>
> 4630f2caafcd ("coredump: get machine check errors early rather than during iov_iter")
Bah. If comes from that
#define dump_page_copy(src,dst) ((dst),(src))
and I did it that way because I wanted to avoid *another* warning,
namely the "dst not used" thing.
But it would have probably been better to either make it an inline
function, or maybe an explicit cast, eg
#define dump_page_copy(src,dst) ((void)(dst),(src))
or whatever.
Linus
Hi all,
On Tue, 5 Mar 2024 18:48:30 -0800 Linus Torvalds <torvalds@linux-foundationorg> wrote:
>
> On Tue, 5 Mar 2024 at 15:51, Stephen Rothwell <[email protected]> wrote:
> >
> > fs/coredump.c: In function 'dump_user_range':
> > fs/coredump.c:923:40: warning: left-hand operand of comma expression has no effect [-Wunused-value]
> > 923 | #define dump_page_copy(src, dst) ((dst), (src))
> > | ^
> > fs/coredump.c:948:58: note: in expansion of macro 'dump_page_copy'
> > 948 | int stop = !dump_emit_page(cprm, dump_page_copy(page, dump_page));
> > | ^~~~~~~~~~~~~~
> >
> > Introduced by commit
> >
> > 4630f2caafcd ("coredump: get machine check errors early rather than during iov_iter")
>
> Bah. If comes from that
>
> #define dump_page_copy(src,dst) ((dst),(src))
>
> and I did it that way because I wanted to avoid *another* warning,
> namely the "dst not used" thing.
>
> But it would have probably been better to either make it an inline
> function, or maybe an explicit cast, eg
>
> #define dump_page_copy(src,dst) ((void)(dst),(src))
>
> or whatever.
This became a build failure for my i386 defconfig build, so I did this:
From: Stephen Rothwell <[email protected]>
Date: Wed, 6 Mar 2024 15:28:12 +1100
Subject: [PATCH] fix up for "coredump: get machine check errors early rather
than during iov_iter"
Signed-off-by: Stephen Rothwell <[email protected]>
---
fs/coredump.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/coredump.c b/fs/coredump.c
index ea155ffee14c..5353b7ac67f2 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -920,7 +920,10 @@ static struct page *dump_page_copy(struct page *src, struct page *dst)
#define dump_page_alloc() ((struct page *)8) // Not NULL
#define dump_page_free(x) do { } while (0)
-#define dump_page_copy(src, dst) ((dst), (src))
+static struct page *dump_page_copy(struct page *src, struct page *dst)
+{
+ return NULL;
+}
#endif
--
2.43.0
--
Cheers,
Stephen Rothwell
On Tue, 5 Mar 2024 at 20:37, Stephen Rothwell <[email protected]> wrote:
>
> +static struct page *dump_page_copy(struct page *src, struct page *dst)
> +{
> + return NULL;
> +}
No, it needs to be "return src;" not NULL.
That
#define dump_page_copy(src, dst) ((dst), (src))
was supposed to be a "use 'dst', return 'src'" macro, and is correct
as that. The problem - as you noticed - is that it causes that "left
side of comma expression has no effect" warning.
(Technically it *does* have an effect - exactly the "argument is used"
one - but the compiler warning does make sense).
Actually, the simplest thing to do is probably just
#define dump_page_free(x) ((void)(x))
#define dump_page_copy(src, dst) (src)
where the "use" of the 'dump_page' argument is that dump_page_free()
void cast, and dump_page_copy() simply doesn't need to use it at all.
Christian?
Linus
Hi all,
On Wed, 6 Mar 2024 15:37:03 +1100 Stephen Rothwell <[email protected]> wrote:
>
> On Tue, 5 Mar 2024 18:48:30 -0800 Linus Torvalds <[email protected]> wrote:
> >
> > On Tue, 5 Mar 2024 at 15:51, Stephen Rothwell <[email protected]> wrote:
> > >
> > > fs/coredump.c: In function 'dump_user_range':
> > > fs/coredump.c:923:40: warning: left-hand operand of comma expression has no effect [-Wunused-value]
> > > 923 | #define dump_page_copy(src, dst) ((dst), (src))
> > > | ^
> > > fs/coredump.c:948:58: note: in expansion of macro 'dump_page_copy'
> > > 948 | int stop = !dump_emit_page(cprm, dump_page_copy(page, dump_page));
> > > | ^~~~~~~~~~~~~~
> > >
> > > Introduced by commit
> > >
> > > 4630f2caafcd ("coredump: get machine check errors early rather than during iov_iter")
> >
> > Bah. If comes from that
> >
> > #define dump_page_copy(src,dst) ((dst),(src))
> >
> > and I did it that way because I wanted to avoid *another* warning,
> > namely the "dst not used" thing.
> >
> > But it would have probably been better to either make it an inline
> > function, or maybe an explicit cast, eg
> >
> > #define dump_page_copy(src,dst) ((void)(dst),(src))
> >
> > or whatever.
>
> This became a build failure for my i386 defconfig build, so I did this:
>
> From: Stephen Rothwell <[email protected]>
> Date: Wed, 6 Mar 2024 15:28:12 +1100
> Subject: [PATCH] fix up for "coredump: get machine check errors early rather
> than during iov_iter"
>
> Signed-off-by: Stephen Rothwell <[email protected]>
> ---
> fs/coredump.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index ea155ffee14c..5353b7ac67f2 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -920,7 +920,10 @@ static struct page *dump_page_copy(struct page *src, struct page *dst)
>
> #define dump_page_alloc() ((struct page *)8) // Not NULL
> #define dump_page_free(x) do { } while (0)
> -#define dump_page_copy(src, dst) ((dst), (src))
> +static struct page *dump_page_copy(struct page *src, struct page *dst)
> +{
> + return NULL;
> +}
>
> #endif
On second thoughts I made it return "src" instead of "NULL";
--
Cheers,
Stephen Rothwell
On Tue, Mar 05, 2024 at 08:47:40PM -0800, Linus Torvalds wrote:
> On Tue, 5 Mar 2024 at 20:37, Stephen Rothwell <[email protected]> wrote:
> >
> > +static struct page *dump_page_copy(struct page *src, struct page *dst)
> > +{
> > + return NULL;
> > +}
>
> No, it needs to be "return src;" not NULL.
>
> That
>
> #define dump_page_copy(src, dst) ((dst), (src))
>
> was supposed to be a "use 'dst', return 'src'" macro, and is correct
> as that. The problem - as you noticed - is that it causes that "left
> side of comma expression has no effect" warning.
>
> (Technically it *does* have an effect - exactly the "argument is used"
> one - but the compiler warning does make sense).
>
> Actually, the simplest thing to do is probably just
>
> #define dump_page_free(x) ((void)(x))
> #define dump_page_copy(src, dst) (src)
>
> where the "use" of the 'dump_page' argument is that dump_page_free()
> void cast, and dump_page_copy() simply doesn't need to use it at all.
>
> Christian?
I would just do it like Stephen did (but returning src ofc) because it's
symmetric to the #ifdef copy_mc_to_kernel definition of dump_page_copy()
and seems easier to read to me.
But I honestly don't care too much. So I'd pick Stephen's change and if
you prefer to do it differently just change it when I send you the pr.
+/*
+ * If we might get machine checks from kernel accesses during the
+ * core dump, let's get those errors early rather than during the
+ * IO. This is not performance-critical enough to warrant having
+ * all the machine check logic in the iovec paths.
+ */
+#ifdef copy_mc_to_kernel
+
+#define dump_page_alloc() alloc_page(GFP_KERNEL)
+#define dump_page_free(x) __free_page(x)
+static struct page *dump_page_copy(struct page *src, struct page *dst)
+{
+ void *buf = kmap_local_page(src);
+ size_t left = copy_mc_to_kernel(page_address(dst), buf, PAGE_SIZE);
+ kunmap_local(buf);
+ return left ? NULL : dst;
+}
+
+#else
+
+/* We just want to return non-NULL; it's never used. */
+#define dump_page_alloc() ERR_PTR(-EINVAL)
+#define dump_page_free(x) ((void)(x))
+static inline struct page *dump_page_copy(struct page *src, struct page *dst)
+{
+ return src;
+}
+#endif