netdev - Re: [PATCH net] qlcnic: Fix mailbox completion handling during spurious interrupt

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

Date:	Mon, 16 Nov 2015 15:30:53 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	rajesh.borundia@...gic.com
Cc:	netdev@...r.kernel.org, Dept-HSGLinuxNICDev@...gic.com
Subject: Re: [PATCH net] qlcnic: Fix mailbox completion handling during
 spurious interrupt

From: Rajesh Borundia <rajesh.borundia@...gic.com>
Date: Mon, 16 Nov 2015 05:58:47 -0500

> +	} else {
> +		if (atomic_read(&mbx->rsp_status) != rsp_status)
> +			qlcnic_83xx_notify_mbx_response(mbx);

Here we go with more of some "We don't understand what atomic_t's
actually do"...

atomic_read() does _nothing_

It doesn't enforce ordering of the read of the variable wrt.  other
memory accesses or anything like that.  It doesn't make the read
"atomic".

All it does is convert an "atomic_t" into an "int", and such a
conversion is necessary if the implementation makes use of some of the
atomic_t value for implementing atomicity (such as putting a spinlock
in the upper byte of the value, etc.)

Therefore, if you are using an atomic_t to order stores and reads of
an integer value.  That's not what they do, and your usage is wrong.

If you are assuming that atomic_t's store a full 32-bit signed
integer, all 32-bits of which which are unmolested by the
implementation, then your usage is wrong.  The implementation may use
any number of bits of the atomic_t value for it's own usage.

As a result, these uses in the qlogic driver are completely wrong and
give a false impression as to what the atomic_read() function does and
what atomic_t variables are to be used for.

Please correct this, and then respin your patch on top of that.
--
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