Re: [PATCH] string: Improve the generic strlcpy() implementation

[Ingo Molnar <mingo@kernel.org>]

* Ingo Molnar <mingo@kernel.org> wrote:

> Hm, so GCC (v4.9.2) will only warn about this bug if -Wtype-limits is enabled 
> explicitly:
> 
>  lib/string.c: In function ‘strlcpy’:
>  lib/string.c:228:32: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>    if (unlikely((size_t)dst_size < 0)) {
>                                  ^
> 
> ... which we don't do in the kernel.
> 
> Has anyone considered enabling -Wtype-limits? It seems to catch real bugs.
> 
> I can see there are patches that enable -Wextra (which enables -Wtype-limits and 
> many other warnings), but it would be more manageable to just enable one such 
> warning at a time.

So I checked this and -Wtype-limits is super chatty at the moment, on various 
configs on x86:

 def64:    warnings:  51, delta: +51
 def32:    warnings:  50, delta: +50
 allno64:  warnings:  24, delta: +21
 allno32:  warnings:  26, delta: +24
 allyes64: warnings: 292, delta: +278
 allyes32: warnings: 318, delta: +277
 allmod64: warnings: 298, delta: +287
 allmod32: warnings: 324, delta: +286

(The delta column is the number of new warnings relative to v4.3-rc4.)

I picked 10 random warnings that triggered in files that looked interesting to me:

1) false positive warning:

./arch/x86/include/asm/apic.h:33:11: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

is caused by macro substitution:

#define apic_printk(v, s, a...) do {       \
                if ((v) <= apic_verbosity) \
                        printk(s, ##a);    \
        } while (0)

so if 'v' is 0 GCC thinks it's a bad comparison - while it isn't.

This would be easily worked around if we moved the code from CPP to C - which is 
beneficial in any case.

2) confused code (possibly harmful):

arch/x86/kernel/pci-calgary_64.c:299:25: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

this:

  static void iommu_free(struct iommu_table *tbl, dma_addr_t dma_addr, unsigned int npages)
  {

	...
        if (unlikely((dma_addr >= DMA_ERROR_CODE) && (dma_addr < badend))) {
                WARN(1, KERN_ERR "Calgary: driver tried unmapping bad DMA "
                       "address 0x%Lx\n", dma_addr);
                return;
        }

is nonsense because dma_addr is unsigned and DMA_ERROR_CODE is 0. The right way to 
check for DMA_ERROR_CODE is to check it:

  triton:~/tip> git grep DMA_ERROR_CODE | grep -E '==|!=' | wc -l
  29

not compare it:

  triton:~/tip> git grep DMA_ERROR_CODE | grep -E ' <= | >= | < | > ' | wc -l
  1

3) false positive warning:

block/cfq-iosched.c:4657:13: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

substition of '0' in the STORE_FUNCTION() macro causes this one.

4) confused code (looks harmless):

crypto/asymmetric_keys/x509_cert_parser.c:549:11: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

        unsigned year, mon, day, hour, min, sec, mon_len;
	...

            hour < 0 || hour > 23 ||

so 'hour' cannot be negative. Looks harmless.

5) false positive warning:

drivers/nvdimm/pmem.c:257:38: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

        if (nvdimm_namespace_capacity(ndns) < ND_PFN_ALIGN

so ND_PFN_ALIGN can be 0 if CONFIG_NVDIMM_PFN is disabled.

6) confused code (looks harmless):

kernel/auditsc.c:1027:23: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

        size_t len, len_left, to_send;

	...

        if (WARN_ON_ONCE(len < 0 || len > MAX_ARG_STRLEN - 1)) {

So this looks like a real bug similar to the one I made in the strlcpy() warning: 
the code wants to warn about a negative underflow - but instead it does not check 
for that condition at all.

It's harmless because the len > MAX_ARG_STRLEN-1 check would catch any underflows.

7) confused code (looks harmless):

mm/memblock.c:840:11: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]


	void __init_memblock __next_reserved_mem_region(u64 *idx,
	...

        if (*idx >= 0 && *idx < type->cnt) {

so *idx is u64 so it's always >= 0. Looks harmless.

8) confused code (looks harmless):

fs/cachefiles/bind.c:42:30: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

        /* start by checking things over */
        ASSERT(cache->fstop_percent >= 0 &&
               cache->fstop_percent < cache->fcull_percent &&
               cache->fcull_percent < cache->frun_percent &&
               cache->frun_percent  < 100);


'fstop_percent' is unsigned int, so the >= 0 test is superfluous. Looks harmless.

9) confused code (looks harmless):

fs/cachefiles/daemon.c:225:14: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

                                       size_t datalen,
	...

        if (datalen < 0 || datalen > PAGE_SIZE - 1)
                return -EOPNOTSUPP;


so 'datalen' is unsigned and this can never be negative - and the second check 
catches any underflows. Looks harmless.

10) somewhat confused code (harmless):

net/rds/iw_recv.c:203:26: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]

So this comes from the comparison of RDS_PAGE_LAST_OFF:

        get_page(recv->r_frag->f_page);

        if (ic->i_frag.f_offset < RDS_PAGE_LAST_OFF) {
                ic->i_frag.f_offset += RDS_FRAG_SIZE;
        } else {
                put_page(ic->i_frag.f_page);
                ic->i_frag.f_page = NULL;
                ic->i_frag.f_offset = 0;
        }

but i_frag.f_offset is unsigned:

 net/rds/iw.h:   unsigned long           f_offset;

and:

 net/rds/iw.h:#define RDS_PAGE_LAST_OFF (((PAGE_SIZE  / RDS_FRAG_SIZE) - 1) * RDS_FRAG_SIZE)

where RDS_FRAG_SIZE == 4096, so RDS_PAGE_LAST_OFF becomes 0.

if RDS_FRAG_SIZE was smaller than 4096, say 512, then RDS_PAGE_LAST_OFF would show 
the offset within the page of the last fragment, i.e. 3584.

So the code looks correct but inefficient: it does the get_page()/put_page() 
unnecessarily in the RDS_FRAG_SIZE == PAGE_SIZE case, which is the default on most 
architectures.

I'd write this as:

        if (ic->i_frag.f_offset < RDS_PAGE_LAST_OFF) {
	        get_page(recv->r_frag->f_page);
                ic->i_frag.f_offset += RDS_FRAG_SIZE;
        } else {
                ic->i_frag.f_page = NULL;
                ic->i_frag.f_offset = 0;
        }

(but this would still generate the warning.)

============

So the summary from 10 examples is:

 1) false positive warning
 2) confused code (possibly harmful)
 3) false positive warning
 4) confused code (looks harmless)
 5) false positive warning
 6) confused code (looks harmless)
 7) confused code (looks harmless)
 8) confused code (looks harmless)
 9) confused code (looks harmless)
10) somewhat confused code (harmless)

I.e. 3 genuine false positive warnings, 6 pointing out harmless looking code 
confusion, 1 pointing out harmful looking code confusion - and one of the sites it 
pointed out highlighted an inefficiency.

That doesn't look too bad of a false positive ration from a compiler warning, 
especially considering that static checkers (and people running -Wextra builds) 
have been cleaning out these cases for quite some time it appears, which means 
these are the leftover warnings.

Thanks,

	Ingo