netdev - Re: [PATCH 1/3] qla3xxx: ethernet: Fix bogus interrupt state flag.

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: <CAOD=uF54P9iZF=gtk6W=53T+857udZbbH_zVfOrDHBUCo=Fh9Q@mail.gmail.com>
Date:	Sat, 3 Mar 2012 15:01:25 +0530
From:	santosh prasad nayak <santoshprasadnayak@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	bhutchings@...arflare.com, jitendra.kalsaria@...gic.com,
	ron.mercer@...gic.com, linux-driver@...gic.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 1/3] qla3xxx: ethernet: Fix bogus interrupt state flag.

On Sat, Mar 3, 2012 at 2:43 AM, David Miller <davem@...emloft.net> wrote:
> From: santosh prasad nayak <santoshprasadnayak@...il.com>
> Date: Fri, 2 Mar 2012 21:24:29 +0530
>
>> In "ql_adapter_initialize", first unlock is done by
>> "spin_unlock_irqrestore(&qdev->hw_lock, hw_flags)"
>> with "hw_flags = 0"   ("hw_flags" is local variable and initialized to
>> zero.),   which is as good as
>> spin_unlock_irq.
>
> You must never pass to irqrestore anything other than a hw_flags
> value given by irqsave or similar.

David,

Thats what my point is.

The function call is as follow:

ql_adapter_up()
{
         .....
   spin_lock_irqsave(&qdev->hw_lock, hw_flags);
            .....
    err = ql_adapter_initialize(qdev);
             .....
   spin_unlock_irqrestore(&qdev->hw_lock, hw_flags);
            ......
}

ql_adapter_initialize()
{
    unsigned long hw_flags = 0;   // D
        .......
   spin_unlock_irqrestore(&qdev->hw_lock, hw_flags);  // A
   msleep(500);                                                         // B
   spin_lock_irqsave(&qdev->hw_lock, hw_flags);        // C
        .....
}

In ql_adapter_initialize, at A,
'spin_unlock_irqrestore'  is called with "hw_flags = 0",
which is as good as spin_unlock_irq().

Static analyzer is showing it as "Error : bogus hw_flags",
which is true. Because "hw_flags" is initialized to zero at D
and the same  "hw_flags" is used to restore IRQ at A.

If intention of the developer is to unlock and enable IRQ
at A then we can use "spin_unlock_irq()"  which will
remove the static analyzer error and also give better
performance.


Regards
Santosh



> You may not assume anything about what values hw_flags takes on nor
> what those values might mean, they are architecture specific so
> you may not just set it to zero and assume that does anything in
> particular.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html