On Thu, Jul 30, 2026 at 03:49:23PM +0530, Mallesh Koujalagi wrote:
> event_string[] has a fixed size of WEDGE_STR_LEN (32) bytes.
> The original scnprintf()-based loop required a manual pre-flight
> bounds check.
>
> Replace the manual bookkeeping with seq_buf, which tracks overflow
> internally. seq_buf_printf() writes each "method," token into the
> buffer.
>
> On overflow, len retains the position of the last
> successful write, so the trailing comma is stripped cleanly without
> including any partial method name in the uevent payload.
>
> Fixes: b7cf9f4ac1b8 ("drm: Introduce device wedged event")
> Signed-off-by: Mallesh Koujalagi <[email protected]>
> ---
> v2:
> - Add proper logic to handle recovery string. (Raag)
>
> v3:
> - Convert manual bounds check to seq_buf. (Jani Nikula)
> - Use drm_WARN_ONCE() instead of drm_WARN_ON() for overflow. (Raag)
>
> v4:
> - Use DECLARE_SEQ_BUF. (Jani)
Make sure to cc the reviewers so that they are in the loop.
> - Warn overflow at end of loop.
> ---
> drivers/gpu/drm/drm_drv.c | 30 ++++++++++++++++++------------
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index e51ed959da89..e429f52ba230 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -36,6 +36,7 @@
> #include <linux/mount.h>
> #include <linux/pseudo_fs.h>
> #include <linux/sched.h>
> +#include <linux/seq_buf.h>
> #include <linux/slab.h>
> #include <linux/sprintf.h>
> #include <linux/srcu.h>
> @@ -574,27 +575,32 @@ static const char *drm_get_wedge_recovery(unsigned int
> opt)
> int drm_dev_wedged_event(struct drm_device *dev, unsigned long method,
> struct drm_wedge_task_info *info)
> {
> - char event_string[WEDGE_STR_LEN], pid_string[PID_STR_LEN],
> comm_string[COMM_STR_LEN];
> - char *envp[] = { event_string, NULL, NULL, NULL };
> - const char *recovery = NULL;
> - unsigned int len, opt;
> + DECLARE_SEQ_BUF(event_string, WEDGE_STR_LEN);
> + char pid_string[PID_STR_LEN], comm_string[COMM_STR_LEN];
> + char *envp[4] = { };
> + unsigned int len = 0, opt;
>
> - len = scnprintf(event_string, sizeof(event_string), "%s", "WEDGED=");
> + seq_buf_puts(&event_string, "WEDGED=");
> + envp[0] = event_string.buffer;
>
> for_each_set_bit(opt, &method, BITS_PER_TYPE(method)) {
> - recovery = drm_get_wedge_recovery(opt);
> + const char *recovery = drm_get_wedge_recovery(opt);
> if (drm_WARN_ONCE(dev, !recovery, "invalid recovery method
> %u\n", opt))
> break;
>
> - len += scnprintf(event_string + len, sizeof(event_string) -
> len, "%s,", recovery);
> + if (!seq_buf_printf(&event_string, "%s,", recovery))
> + len = seq_buf_used(&event_string);
Should we continue iterating if seq_buf_printf() fails?
Also, why not just
if (drm_WARN_ON_ONCE(dev, !seq_buf_printf(&event_string, "%s,",
recovery)))
break;
and drop seq_buf_has_overflowed() below?
Raag
> }
>
> - if (recovery)
> - /* Get rid of trailing comma */
> - event_string[len - 1] = '\0';
> + drm_WARN_ONCE(dev, seq_buf_has_overflowed(&event_string),
> + "WEDGED event string truncated\n");
> +
> + if (len)
> + /* Strip trailing comma; also discards any partial overflow
> entry */
> + event_string.buffer[len - 1] = '\0';
> else
> - /* Caller is unsure about recovery, do the best we can at this
> point. */
> - snprintf(event_string, sizeof(event_string), "%s",
> "WEDGED=unknown");
> + /* No complete entry written, do the best we can at this point.
> */
> + snprintf(event_string.buffer, event_string.size, "%s",
> "WEDGED=unknown");
>
> drm_info(dev, "device wedged, %s\n", method == DRM_WEDGE_RECOVERY_NONE ?
> "but no recovery needed" : "needs recovery");
> --
> 2.48.1
>
