linux-kernel - Re: [PATCH 2/4] libata: Implement disk shock protection support

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]

Date:	Thu, 11 Sep 2008 00:56:42 +0200
From:	Tejun Heo <htejun@...il.com>
To:	Elias Oltmanns <eo@...ensachen.de>
CC:	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>,
	Jeff Garzik <jeff@...zik.org>,
	Randy Dunlap <randy.dunlap@...cle.com>,
	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] libata: Implement disk shock protection support

Elias Oltmanns wrote:
>>> Should I change ata_eh_about_to_do() so that it will record the action
>>> in link->eh_context before clearing it in link->eh_info?
>> That's what ata_eh_about_to_do() currently does, exactly.  Actually,
>> that's the whole reason it's there as the described problem exists for
>> all other actions too.  :-)
> 
> Sounds reasonable enough. Much as I regret it, though, I really can't
> find that this is what actually happens. Where exactly is the action
> propagated from ehi to ehc->i? (Checked next-20080903, v2.6.27-rc5 and
> v2.6.26).

Oops, that was me being stupid.  I can't find it either.  Right, it's
never pulled in as for all other actions, it's enough to make sure
that EH is repeated if an action gets scheduled after
ata_eh_about_to_do().  Sorry about the confusion.  Can you please use
the following function before ata_eh_about_to_do()?

static void ata_eh_pull_action(struct ata_link *link, struct ata_device
*dev,
			       unsigned int action)
{
	...
	struct ata_eh_info *ehi = &link->eh_info;
	struct ata_eh_context *ehc = &link->eh_context;
	...

	spin_lock_irqsave(ap->lock, flags);

	if (dev)
		ehc->i.dev_action[dev->devno] |=
			ehi->dev_action[dev->devno] & action;
	ehc->i.action |= ehi->action & action;

	spin_unlock_irqrestore(ap->lock, flags);
}

And add comment explaning why the operation is needed for unload
action?

> On another matter: I don't particularly like the idea that there should
> appear an "EH complete" in the logs every time a head unload request has
> been processed. Is it safe to set ATA_EHI_QUIET when scheduling unload
> requests or is the risk that something important may be missed too high?

Hmmm... ATA_EHI_QUIET masks all EH reporting and as error conditions
are not unlikely under physical shocks, I don't think suppressing them
all is a good idea.  How about adding ATA_EH_QUIET_MASK or a boolean
parameter to ata_eh_about_to_do() such that unload action doesn't set
RECOVERED flag?

Thanks.  :-)

-- 
tejun
--
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/