linux-kernel - Re: [PATCH v2] drm/xe: Fix build error for XE_IOCTL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKbEznuRLDwDhuYi3WwWR7GWbvu0oBCB53UF_4Yr+FpScYmWDA@mail.gmail.com>
Date: Sat, 2 Nov 2024 09:01:02 +0900
From: gyeyoung <gye976@...il.com>
To: Lucas De Marchi <lucas.demarchi@...el.com>
Cc: Oded Gabbay <ogabbay@...nel.org>, 
	Thomas Hellström <thomas.hellstrom@...ux.intel.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, 
	intel-xe@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drm/xe: Fix build error for XE_IOCTL_DBG macro

> >--- a/drivers/gpu/drm/xe/xe_macros.h
> >+++ b/drivers/gpu/drm/xe/xe_macros.h
> >@@ -11,8 +11,12 @@
> > #define XE_WARN_ON WARN_ON
> >
> > #define XE_IOCTL_DBG(xe, cond) \
> >-      ((cond) && (drm_dbg(&(xe)->drm, \
> >-                          "Ioctl argument check failed at %s:%d: %s", \
> >-                          __FILE__, __LINE__, #cond), 1))
> >+({ \
> >+      if ((cond)) \
> >+              drm_dbg(&(xe)->drm, \
> >+                      "Ioctl argument check failed at %s:%d: %s", \
> >+                      __FILE__, __LINE__, #cond); \
> >+      (cond); \
>
> there's a double cond evaluation here and given any expression can be
> given to XE_IOCTL_DBG(), this doens't look very safe. I think this would
> be safer as:
>
> #define XE_IOCTL_DBG(xe, cond) ({                                       \
>          int cond__ = !!(cond);                                          \
>          if (cond__)                                                     \
>                  drm_dbg(&(xe)->drm,                                     \
>                          "Ioctl argument check failed at %s:%d: %s",     \
>                          __FILE__, __LINE__, #cond);                     \
>          cond__;                                                         \
> })
>
> as it then evaluates cond just once. Also the generated code seems to be
> sane compared to what we had before too.
>

Yes, if (cond) has operator like ++, it will be a bug. I miss that...
I will revise a patch again by referring to your review, thanks.

> And I also needed this to build-test:
>
> | diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> | index 08cfea04e22bd..82585d442f017 100644
> | --- a/drivers/gpu/drm/drm_print.c
> | +++ b/drivers/gpu/drm/drm_print.c
> | @@ -215,9 +215,8 @@ void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf)
> |  {
> |         const struct drm_device *drm = p->arg;
> |         const struct device *dev = drm ? drm->dev : NULL;
> | -       enum drm_debug_category category = p->category;
> |
> | -       if (!__drm_debug_enabled(category))
> | +       if (!__drm_debug_enabled(p->category))
> |                 return;
> |
> |         __drm_dev_vprintk(dev, KERN_DEBUG, p->origin, p->prefix, vaf);
>
> as otherwise it complains category is unused.
>

I also submitted a seperate patch to fix '__drm_debug_enabled' macro,
from '#define __drm_debug_enabled(category)  true'
to     '#define __drm_debug_enabled(category)  ({ void(category);  true; })'
This removes the build error caused by the unused 'category', too.

Anyway, it can be build. I tested both cases.
I realize now that these two patches should have been submitted as a
patch series
I'm sorry for my mistakes.

Thanks,
Gyeyoung Baek

> Lucas De Marchi