linux-kernel - Re: [PATCH 7/8] IPMI: convert locked counters to atomics

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: <20080213152645.9e589d0c.akpm@linux-foundation.org>
Date:	Wed, 13 Feb 2008 15:26:45 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	minyard@....org
Cc:	linux-kernel@...r.kernel.org,
	openipmi-developer@...ts.sourceforge.net
Subject: Re: [PATCH 7/8] IPMI: convert locked counters to atomics

On Wed, 13 Feb 2008 10:32:20 -0600
Corey Minyard <minyard@....org> wrote:

> From: Konstantin Baydarov <kbaidarov@...mvista.com>
> 
> Atomics are a lot more efficient and neat than using a lock.
>   

Yes, but...

> +struct ipmi_stats
> +{
> +	/* Commands we got that were invalid. */
> +	atomic_t sent_invalid_commands;
> +
> +	/* Commands we sent to the MC. */
> +	atomic_t sent_local_commands;
> +	/* Responses from the MC that were delivered to a user. */
> +	atomic_t handled_local_responses;
> +	/* Responses from the MC that were not delivered to a user. */
> +	atomic_t unhandled_local_responses;
> +
> +	/* Commands we sent out to the IPMB bus. */
> +	atomic_t sent_ipmb_commands;
> +	/* Commands sent on the IPMB that had errors on the SEND CMD */
> +	atomic_t sent_ipmb_command_errs;
> +	/* Each retransmit increments this count. */
> +	atomic_t retransmitted_ipmb_commands;
> +	/* When a message times out (runs out of retransmits) this is
> +           incremented. */
> +	atomic_t timed_out_ipmb_commands;
> +
> +	/* This is like above, but for broadcasts.  Broadcasts are
> +           *not* included in the above count (they are expected to
> +           time out). */
> +	atomic_t timed_out_ipmb_broadcasts;
> +
> +	/* Responses I have sent to the IPMB bus. */
> +	atomic_t sent_ipmb_responses;
> +
> +	/* The response was delivered to the user. */
> +	atomic_t handled_ipmb_responses;
> +	/* The response had invalid data in it. */
> +	atomic_t invalid_ipmb_responses;
> +	/* The response didn't have anyone waiting for it. */
> +	atomic_t unhandled_ipmb_responses;
> +
> +	/* Commands we sent out to the IPMB bus. */
> +	atomic_t sent_lan_commands;
> +	/* Commands sent on the IPMB that had errors on the SEND CMD */
> +	atomic_t sent_lan_command_errs;
> +	/* Each retransmit increments this count. */
> +	atomic_t retransmitted_lan_commands;
> +	/* When a message times out (runs out of retransmits) this is
> +           incremented. */
> +	atomic_t timed_out_lan_commands;
> +
> +	/* Responses I have sent to the IPMB bus. */
> +	atomic_t sent_lan_responses;
> +
> +	/* The response was delivered to the user. */
> +	atomic_t handled_lan_responses;
> +	/* The response had invalid data in it. */
> +	atomic_t invalid_lan_responses;
> +	/* The response didn't have anyone waiting for it. */
> +	atomic_t unhandled_lan_responses;
> +
> +	/* The command was delivered to the user. */
> +	atomic_t handled_commands;
> +	/* The command had invalid data in it. */
> +	atomic_t invalid_commands;
> +	/* The command didn't have anyone waiting for it. */
> +	atomic_t unhandled_commands;
> +
> +	/* Invalid data in an event. */
> +	atomic_t invalid_events;
> +	/* Events that were received with the proper format. */
> +	atomic_t events;
> +};

The code forgot to initialise all of these.

It just so happens that the all-bits-zero pattern works correctly for all
current architectures, so the code should work OK.  But there is no reason
(I hope) why an architecture cannot implement atomic_t as

struct atomic_t {
	int counter;
	spinlock_t lock;
};

in which case the results of ATOMIC_INIT() may _not_ be all-zeroes, in
which case the code will deadlock.

So.  It works, but it's grubby.   Do you still wish to proceed?
--
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/