linux-kernel - Re: kfifo must_check warning (+ patch)

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: <kfifo-patch1-reply@mdm.bga.com>
Date:	Wed, 20 Oct 2010 12:52:28 -0500
From:	miltonm@....com
To:	Randy Dunlap <randy.dunlap@...cle.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	lkml <linux-kernel@...r.kernel.org>,
	Stefani Seibold <stefani@...bold.net>
Subject: Re: kfifo must_check warning (+ patch)


> --- linux-next-20101019.orig/drivers/char/n_gsm.c
> +++ linux-next-20101019/drivers/char/n_gsm.c
> @@ -1573,11 +1573,14 @@  static void gsm_dlci_command(struct gsm_
>  static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
>  {
>  	struct gsm_dlci *dlci = kzalloc(sizeof(struct gsm_dlci), GFP_ATOMIC);
> +	int err;
> +
>  	if (dlci == NULL)
>  		return NULL;
>  	spin_lock_init(&dlci->lock);
>  	dlci->fifo = &dlci->_fifo;
> -	if (kfifo_alloc(&dlci->_fifo, 4096, GFP_KERNEL) < 0) {
> +	err = kfifo_alloc(&dlci->_fifo, 4096, GFP_KERNEL);
> +	if (err < 0) {
>  		kfree(dlci);
>  		return NULL;
>  	}
> 
> 


I think this is not a gcc bug but Randy's gcc beeing smarter
and showing a signed bug with the old code.

The earlier discussion had:

> > > Should the driver code be doing something else?
> > > or should the kfifo_alloc() macro be checking the result of the helper?
> > > 
> > 
> > A gcc bug, I'd say.  The code looks OK and my gcc doesn't warn.
> 
> OK.
> 
> > Perhaps see if you can find some code transformation in n_gsm.c which
> > makes it go away?  Add a new local variable or something.
> 
> Yes, that works for me.  Patch is below.
> 

> > I did see a probably unrelated bug in there though. 
> > __kfifo_must_check_helper() coerces its arg into an `unsigned int' and
> > returns an unsigned int.  Consequently if kfifo_alloc() tries to return
> > -EINVAL, the above-quoted code won't detect the error: it will see
> > -EINVAL as a large, positive unsigned value.
> > 

and then gcc decides that the check of < 0 is meaningless, and then
says that the result is unused.

Note: I'm a kernel hacker , not a gcc one.  This is pure speculation.

So to me the helper is the one causing the error!  ... and the patch is
undoing the damage at the caller.

So the correct fix is to change the return type of the helper,
and if someone wants the int to be unsigned then it needs to be
unsigned long so we can use an is_error type test.

milton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/