linux-kernel - Re: [PATCH v9 4/4] drm/panthor: show device-wide list of DRM GEM objects over DebugFS

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250418100454.788c9586@collabora.com>
Date: Fri, 18 Apr 2025 10:04:54 +0200
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Adrián Larumbe <adrian.larumbe@...labora.com>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 kernel@...labora.com, Liviu Dudau <liviu.dudau@....com>, Steven Price
 <steven.price@....com>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, Sumit Semwal <sumit.semwal@...aro.org>,
 Christian König <christian.koenig@....com>,
 linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH v9 4/4] drm/panthor: show device-wide list of DRM GEM
 objects over DebugFS

On Fri, 18 Apr 2025 03:27:07 +0100
Adrián Larumbe <adrian.larumbe@...labora.com> wrote:

> +#ifdef CONFIG_DEBUG_FS
> +static void
> +panthor_gem_debugfs_format_flags(char flags_str[], int flags_len,
> +				 const char * const names[], u32 name_count,
> +				 u32 flags)
> +{
> +	bool first = true;
> +	int offset = 0;
> +
> +#define ACC_FLAGS(...) \
> +	({ \
> +		offset += snprintf(flags_str + offset, flags_len - offset, ##__VA_ARGS__); \
> +		if (offset == flags_len) \
> +			return; \
> +	})

I tried applying, but checkpatch complains with:

-:225: WARNING:MACRO_WITH_FLOW_CONTROL: Macros with flow control statements should be avoided
#225: FILE: drivers/gpu/drm/panthor/panthor_gem.c:359:
+#define ACC_FLAGS(...) \
+	({ \
+		offset += snprintf(flags_str + offset, flags_len - offset, ##__VA_ARGS__); \
+		if (offset == flags_len) \
+			return; \
+	})

and now I'm wondering why we even need to return early? Oh, and the
check looks suspicious to: snprintf() returns the number of chars
that would have been written if the destination was big enough, so
the offset will actually be greater than flags_len if the formatted
string doesn't fit.

There are a few other formatting issues reported by checkpatch
--strict BTW.

Unfortunately this led me to have a second look at these bits
and I have a few more comments, sorry about that :-/.

> +
> +	ACC_FLAGS("%c", '(');

Now that flags have their own columns, I'm not sure it makes sense
surround them with parenthesis. That's even weird if we start running
out of space, because there would be an open '(', a few flags,
the last one being truncated, and no closing ')'. The other thing
that's bothering me is the fact we don't know when not all flags
have been displayed because of lack of space.


> +
> +	if (!flags)
> +		ACC_FLAGS("%s", "none");
> +
> +	while (flags) {
> +		u32 bit = fls(flags) - 1;

I would probably print flags in bit position order, so ffs()
instead of fls().

> +		u32 idx = bit + 1;

Why do you have a "+ 1" here? Feels like an off-by-one error to
me.

> +
> +		if (!first)
> +			ACC_FLAGS("%s", ",");
> +
> +		if (idx < name_count && names[idx])
> +			ACC_FLAGS("%s", names[idx]);
> +
> +		first = false;
> +		flags &= ~BIT(bit);
> +	}
> +
> +	ACC_FLAGS("%c", ')');
> +
> +#undef ACC_FLAGS
> +}

How about something like that:

static void
panthor_gem_debugfs_format_flags(char *flags_str, u32 flags_str_len,
                                 const char * const flag_names[],
                                 u32 flag_name_count, u32 flags)
{
	int offset = 0;

	if (!flags) {
        	snprintf(flags_str, flags_str_len, "%s", "none");
		return;
	}

	while (flags) {
		const char *flag_name = "?";
		u32 flag = ffs(flags) - 1;
		int new_offset = offset;

		flags &= ~BIT(flag);

		if (flag < flag_name_count && flag_names[flag])
			flag_name = flag_names[flag];

		/* Print as much as we can. */
		new_offset += snprintf(flags_str + offset, flags_str_len - offset,
				       "%s%s", offset ? "," : "", flag_name);

		/* If we have flags remaining, check that we have enough space for
		 * the "...".
		 */
		if (flags)
			new_offset += strlen(",...");

		/* If we overflowed, replace what we've written by "..." and
		 * bail out.
		 */
		if (new_offset > flags_str_len) {
			snprintf(flags_str + offset, flags_str_len - offset,
				 "%s...", offset ? "," : "");
			return;
		}

		offset = new_offset;
        }
}