Date: Sat, 6 Aug 2011 13:51:37 +0100 From: "Steven Hartland" <killing@multiplay.co.uk> To: "Jeremy Chadwick" <freebsd@jdc.parodius.com> Cc: freebsd-fs@FreeBSD.ORG Subject: Re: Questions about erasing an ssd to restore performance under FreeBSD Message-ID: <42162705FC5E4E748A1A57285AECA49A@multiplay.co.uk> References: <20110728012437.GA23430@icarus.home.lan> <FD3A11BEFD064193AA24C1DF09EDD719@multiplay.co.uk> <20110728103234.GA33275@icarus.home.lan> <A6828B6CE6764E13A44B1ABF61CF3FED@multiplay.co.uk> <20110728145917.GA37805@icarus.home.lan> <2A07CD8AE6AE49A5BAED59A7E547D1F9@multiplay.co.uk> <2D117F9F212A4CCBA6B7F51E8705BDB7@multiplay.co.uk> <20110805033001.GA47366@icarus.home.lan> <20110805044725.GA48395@icarus.home.lan> <C8CAF8EE63BF41B9A71C987F6A6ACB4C@multiplay.co.uk> <20110806041822.GA11439@icarus.home.lan>
next in thread | previous in thread | raw e-mail | index | archive | help
----- Original Message ----- From: "Jeremy Chadwick" <freebsd@jdc.parodius.com> > 1) Focusing on Erase or Enhanced Erase, it appears you set the internal > timeout to the number of seconds that matches what the device itself > claims to be the duration for the erase to complete (assuming > ident_buf->erase_time is non-zero, else you default to 2 hours). I may > be misunderstanding the timeout specifier here, but based on what I've > read there is no 1:1 correlation between the actual operation timeout > and what the drive claims to be the duration of the erase. Please read > this entry: > > https://ata.wiki.kernel.org/index.php/ATA_Secure_Erase#Command_time-out_during_erase_with_larger_drives > According to the spec the values mean the following:- 0 = unspecified 1-254 = (value*2) minutes 255 = >508 minutes > About general timeouts: the hdparm maintainer just recently (September > 2010) changed his default timeout value from 5 seconds to 15 seconds, > citing that some drives take longer than others. Your patch uses a > timeout of 5 seconds, I would advocate increasing that to 15 "just in > case". Changed longer I suspect is generally safer as you say. > 2) The code logic for setting the master password differs from what > Linux hdparm has. Yours says "if revision isn't zero, and revision > isn't 0xffff, and revision-1 isn't zero, set revision to 0xfffe": > > if (0 != pwd.revision && 0xffff != pwd.revision && 0 == --pwd.revision) { > pwd.revision = 0xfffe; > } The logic here is:- if not 0 and not 0xffff then decrement if decrement goes to 0 set it 0xffffe The reason for doing this is that drives which support this options have the value initialised to 0xffffe so decrementing makes more sence than incrementing. The same wrapping is achieved just in a simpler block. > i) What purpose the 0 != --pwd.revision comparison serves, If the decrement hits 0 wrap it to prevent it hitting 0 otherwise it would leave the drive in a state which indicates "not supported" > ii) Why Linux chooses to increment revision rather than set it to > something statically. They increment this decrments, I beleive the important thing is change, which they both achieve. > The ATA specification doesn't state what needs to be done here (I'm not > surprised). As such, I believe (i) to be unnecessary, and the answer to > (ii) is that Linux may be doing this to ensure compatibility with some > model of drives. I can't confirm (ii) because the author of hdparm > chooses not to use a VCS; he simply uses SourceForge to distribute > tarballs. What I'm saying is that there's no source code annotation > that I can get at, so I can't find the justification behind the logic in > hdparm. Infuriating. >From what I can tell its just mechanisum for external tools to tell if the master password is the same one they encountered last time, hence the increment or decrement, as that ensures its changing on each master password set. > 3) The ata_security_password struct contents are passed directly to the > device as the CDB payload. My concern with this has to do with > big-endian architectures; u_int16_t are used in this struct, and I > would imagine the byte order would be different based on architecture. > Unless CAM does something magical under the hood...? Disk hardware I suspect is one or the other not a mix like cpus, I'm unsure, but I dont see any other manipulation of endian in any other tools. Would need someone who knows to comment. > 4) Do we really need atasecurity_print_time()? (Yes I understand why it > exists per se, since it's called twice, but still...) Don't like duplicate code personaly ;-) > 5) This can be shortened to use ^= instead: > > pwd.ctrl = pwd.ctrl ^ ATA_SECURITY_PASSWORD_MASTER; Of course changed > > 6) I don't know if use of long getopt arguments violates or upsets > folks. It doesn't bother me, but it would make the utility seem > inconsistent ("everything takes single character switches except this > one command, what's up with that?") Given the nature of the options I would personally prefer they where understandable over shorter. It could be changed but I'd prefer not to. > > 7) There are some style(9) issues. The most notable one is that there's > inconsistent use of braces on single-operation if() statements. > style(9) insists for single-ops braces not be used. (An example would > be the above pwd.revision stuff). IIRC it stats they can > > 8) Code syntax/style differences vs. what's already in the utility. I > don't know how much this matters to folks nor do I know what the > Project's stance is on stuff like that. Something tells me there is no > stance, as for example there's mixed-use of printf() and fprintf(stdout) > in camcontrol (the fprintf() makes it more obvious since there are calls > to fprintf(stderr), though those should probably use warnx()). Yes I noticed that the code, and went with warnx and fprintf(stdout looks like its evolved over time. If there is a preference I'm happy to change. > 9) Long argument syntax is inconsistent; usage syntax in code uses > double hyphens (--arg) while man page uses single (-arg). I know both > work, but generally documentation and usage syntax should show double. thats just how man works, if you look at the output it actually prints --option not -option. > 10) Can we get rid of MINIMALISTIC? :-) I had a gut feeling it was to > decrease utility size to minimise space on floppies and I was right: > http://www.freebsd.org/cgi/cvsweb.cgi/src/sbin/camcontrol/camcontrol.c#rev1.37 > Floppy builds were removed with FreeBSD 8.0, so I'm of the opinion all > the #ifndef/#endifs can be removed, and the Makefile updated too. This > might have to wait until RELENG_7 is officially EOL'd though (February > 2013) else any changes to camcontrol in one tag have to be manually > back-ported to an earlier tag. Quite possibly but should be done as a seperate commit imo to make it clear, hence maintaining the compat with the current convention in the patch. >> Some more details and usage examples and caveats can be found here:- >> http://blog.multiplay.co.uk/2011/08/freebsd-security-support-for-ata-devices-via-camcontrol/ >> >> I've updated the code as well as the man pages so everything should be good. >> >> I've not tested all of the various combinations totally yet, but have tested >> all the big ones inc secure erase, set pass, set level, set user & disable. >> >> It should be noted that this requires disks attached to an ATA controller e.g. >> ahci as ATA commands don't appear to pass through other controllers e.g. mpt >> even with ATA disks underneath. > > I imagine the pass-through command "stuff" will need to be addressed in > each respective driver (mps(4), etc.)). I do see mps mention passthough but it doesnt seem to work even with just identify after I switched the logic around to default to ATA instead of ATAPI. Need to do some more investigation. > As for "requiring AHCI mode" (sorry if I misread what you meant), that's > nonsense. I keep having battles with people online who insist that > features like Secure Erase and TRIM don't work unless you use AHCI. For > Secure Erase I know for a fact this is utter nonsense because I've done > a Secure Erase on Windows (using Intel's SSD Toolbox) with my controller > in "Enhanced SATA" (non-AHCI) mode. ATA is ATA, no matter if you're > speaking via legacy PATA or AHCI. So I just want to make that clear to > folks who might come across this post via Google or what not. I don't mean that its needed I just mean it needs ATA command compatiblity hence the "ATA controller such as ahci" with the key word being "such" ;-) >> I'd be interested to here from anyone who has an info on getting this to >> work as well. >> >> Much credit to Daniel Roethlisberger for his work which was the basis >> of this code. This can be found here:- >> http://www.roe.ch/ATA_Security >> http://www.freebsd.org/cgi/query-pr.cgi?pr=bin/127918 > > Someone may want to do the same review of those atacontrol(8) > modifications as those mentioned above. Unfortunately it was never committed, due to the freeze on ata with the migration to cam apparently. I've updated the patch file with the changes mentioned above. Thanks for doing the review most appreciated :) Regards Steve ================================================ This e.mail is private and confidential between Multiplay (UK) Ltd. and the person or entity to whom it is addressed. In the event of misdirection, the recipient is prohibited from using, copying, printing or otherwise disseminating it or any information contained in it. In the event of misdirection, illegible or incomplete transmission please telephone +44 845 868 1337 or return the E.mail to postmaster@multiplay.co.uk.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?42162705FC5E4E748A1A57285AECA49A>