Re: [PATCH v3] checkpatch: Check for missing sentinels in ID arrays

[Brian Norris <briannorris@chromium.org>]

Hi Joe,

On Wed, Jul 02, 2025 at 02:19:45AM -0700, Joe Perches wrote:
> On Tue, 2025-07-01 at 11:34 -0700, Brian Norris wrote:
> > All of the ID tables based on <linux/mod_devicetable.h> (of_device_id,
> > pci_device_id, ...) require their arrays to end in an empty sentinel
> > value. That's usually spelled with an empty initializer entry (e.g.,
> > "{}"), but also sometimes with explicit 0 entries, field initializers
> > (e.g., '.id = ""'), or even a macro entry (like PCMCIA_DEVICE_NULL).
> > 
> > Without a sentinel, device-matching code may read out of bounds.
> > 
> > I've found a number of such bugs in driver reviews, and we even
> > occasionally commit one to the tree. See commit 5751eee5c620 ("i2c:
> > nomadik: Add missing sentinel to match table") for example.
> > 
> > Teach checkpatch to find these ID tables, and complain if it looks like
> > there wasn't a sentinel value.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -685,6 +685,64 @@ our $tracing_logging_tags = qr{(?xi:
> >  	[\.\!:\s]*
> >  )};
> >  
> > +# Device ID types from include/linux/mod_devicetable.h.
> > +our $dev_id_types = qr{(?x:
> 
> [ a long list ...]
> 
> > +)_device_id};
> 
> This list seems unmaintainable.

That seems debatable to me, as we've only had ~20 additions in the last
decade:

$ git log --oneline --since='10 years' -S '_device_id' ./include/linux/mod_devicetable.h | wc -l
20

It's pretty easy to script too. But anyway, I'm not the maintainer, so
I can try your suggestions.

> Does something like '\b[a-z]\w*_device_id\b'
> match too many other non-sentinal uses?
> 
> $ git grep -P -o -h '\b\w+_device_id\b' -- '*.[ch]' | sort | uniq | wc -l
> 288

Just inspecting the tree for struct types that match *_device_id, I see
that there are 21 of them that are not in mod_devicetable.h, and:

* the large majority of them are of the same sentinel style, and
  probably should be in mod_devicetable.h anyway
* of the relatively few that are not quite the same style, there's:
  - struct gio_device_id, which oddly uses '0xff' as a sentinel
  - struct ddb_device_id, which is confined to 1 file, and uses
    ARRAY_SIZE(). This triggers a false positive.
  - a few types like struct pcan_ufd_device_id that might technically
    match, but don't have the same sorts of tables and so are benign
* running checkpatch over the source tree only shows ~1 additional
  false positive; the aforementioned struct ddb_device_id

So on the whole, it's probably not too much of a win to enumerate a list
of types, and I'll just go with your regex instead.

> > @@ -7678,6 +7736,30 @@ sub process {
> []
> > +# Check that *_device_id tables have sentinel entries.
> > +		if (defined $stat && $line =~ /struct $dev_id_types .*\[\] = \{/) {
> 
> Spacing isn't guaranteed so perhaps
> 
> $line =~ /struct\s+$dev_id_types\s+\w+\s*\[\s*\]\s*=\s*\{/

Sure.

Brian