linux-kernel - Re: [PATCH 09/11] firewire-sbp-target: Add sbp_target

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: <20120414133322.6f601eb9@stein>
Date:	Sat, 14 Apr 2012 13:33:22 +0200
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	Chris Boot <bootc@...tc.net>
Cc:	linux1394-devel@...ts.sourceforge.net,
	target-devel@...r.kernel.org, linux-kernel@...r.kernel.org,
	agrover@...hat.com, clemens@...isch.de, nab@...ux-iscsi.org
Subject: Re: [PATCH 09/11] firewire-sbp-target: Add sbp_target_agent.{c,h}

On Apr 14 Stefan Richter wrote:
> On Apr 11 Chris Boot wrote:
> > +static int tgt_agent_rw_agent_state(struct fw_card *card, int tcode, void *data,
> > +		struct sbp_target_agent *agent)
> > +{
> > +	__be32 state;
> > +
> > +	switch (tcode) {
> > +	case TCODE_READ_QUADLET_REQUEST:
> > +		pr_debug("tgt_agent AGENT_STATE READ\n");
> > +
> > +		spin_lock_bh(&agent->lock);
> > +		state = cpu_to_be32(agent->state);
> > +		spin_unlock_bh(&agent->lock);
> > +		memcpy(data, &state, sizeof(state));
> > +
> > +		return RCODE_COMPLETE;
> > +
> > +	case TCODE_WRITE_QUADLET_REQUEST:
> > +		/* ignored */
> > +		return RCODE_COMPLETE;
> > +
> > +	default:
> > +		return RCODE_TYPE_ERROR;
> > +	}
> > +}
> 
> agent->state is an int; reading an int is atomic.  Hence the access on
> its own does not benefit from lock acquisition.

Actually this is only true because all write accesses to agent->state are
merely assignments (not increments or the like).  And I have to admit that
I don't remember whether this is only how compilers work in practice or it
is actually required by the C language specification.

> The memcpy can be simplified to
> 
> 	*(__be32 *)data = cpu_to_be32(agent->state);
> 
> if data is always at least quadlet aligned.  Thy type cast is only to tell
> code checkers like sparse that we know what we are doing.

So unless there is such an atomicity guarantee, leave the locking as is
and prefer

	int state;

	spin_lock_bh(&agent->lock);
	state = agent->state;
	spin_unlock_bh(&agent->lock);

	*(__be32 *)data = cpu_to_be32(state);

> If data may be
> unaligned, you could use
> 
> 	put_unaligned_be32(agent->state, data);

OK, I read further.  This is part of your handler.address_callback.
data will point to quadlet aligned memory then.  It is no where written as
an API specification, but you may rest assured that firewire-core will
always align quadlet read or block read response buffers at least on
quadlet boundary.  You can safely cast data into an u32 or __be32 pointer.
-- 
Stefan Richter
-=====-===-- -=-- -===-
http://arcgraph.de/sr/
--
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/