linux-kernel - Re: [PATCH v2 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: <20120215222741.6b7388dc@stein>
Date:	Wed, 15 Feb 2012 22:27:41 +0100
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 v2 09/11] firewire-sbp-target: Add
 sbp_target_agent.{c,h}

On Feb 15 Chris Boot wrote:
> --- /dev/null
> +++ b/drivers/target/sbp/sbp_target_agent.c
[...]
> +static int tgt_agent_rw_orb_pointer(struct fw_card *card,
> +	int tcode, int generation, void *data,
> +	struct sbp_target_agent *agent)
> +{
> +	struct sbp2_pointer *ptr = data;
> +	int ret;
> +
> +	switch (tcode) {
> +	case TCODE_WRITE_BLOCK_REQUEST:
> +		smp_wmb();
> +		atomic_cmpxchg(&agent->state,
> +				AGENT_STATE_RESET, AGENT_STATE_SUSPENDED);
> +		smp_wmb();
> +		if (atomic_cmpxchg(&agent->state,
> +					AGENT_STATE_SUSPENDED,
> +					AGENT_STATE_ACTIVE)
> +				!= AGENT_STATE_SUSPENDED)
> +			return RCODE_CONFLICT_ERROR;
> +		smp_wmb();

Why the double state change?

And as asked at the patch, which writes are the barriers meant to order,
and how does the corresponding read side look like?  Or are these barriers
not actually needed after all?

[...]
> +void sbp_target_agent_unregister(struct sbp_target_agent *agent)
> +{
> +	if (atomic_read(&agent->state) == AGENT_STATE_ACTIVE)
> +		flush_work_sync(&agent->work);
> +
> +	fw_core_remove_address_handler(&agent->handler);
> +	kfree(agent);
> +}

So, asking once more without having read the code in full yet:  Are you
sure that agent->state is not going to change anymore after you tested it
here?
-- 
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/