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: <20120218155910.72ab30ca@stein>
Date:	Sat, 18 Feb 2012 15:59:10 +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 16 Chris Boot wrote:
> On 15/02/2012 21:27, Stefan Richter wrote:
> > 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?
> 
> Because the SBP spec differentiates between the RESET state, which 
> happens after the agent initialises or is sent an explicit reset 
> request, and when it's suspended between requests...

OK, right, there are the state transitions Reset-->Active and
Suspended-->Active.  Though you implement the former as a swift
Reset-->Suspended-->Active.  Which does indeed work, provided that there
is no other concurrent context which could transition from Suspended to
Anything-but-Active.

> > 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?
> 
> ...of course this is another time when my use of atomics and memory 
> barriers is entirely wrong. I should most likely be using locking here.
> 
> > [...]
> >> +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?
> 
> Nope. At least in this case I can unregister the address handler before 
> I check if I need to flush the work item.

Yep, first unregister the handler, then wait for the work to finish, then
free the data.

And as discussed off-list today, firewire-core should be improved to
guarantee you that the handler isn't still running anywhere when
fw_core_remove_address_handler() returns.
-- 
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/