From owner-freebsd-fs@FreeBSD.ORG Sat Aug 6 04:18:26 2011 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 87E50106567A for ; Sat, 6 Aug 2011 04:18:26 +0000 (UTC) (envelope-from jdc@koitsu.dyndns.org) Received: from qmta08.westchester.pa.mail.comcast.net (qmta08.westchester.pa.mail.comcast.net [76.96.62.80]) by mx1.freebsd.org (Postfix) with ESMTP id 2E6F98FC1F for ; Sat, 6 Aug 2011 04:18:25 +0000 (UTC) Received: from omta15.westchester.pa.mail.comcast.net ([76.96.62.87]) by qmta08.westchester.pa.mail.comcast.net with comcast id GsDG1h0041swQuc58sJS00; Sat, 06 Aug 2011 04:18:26 +0000 Received: from koitsu.dyndns.org ([67.180.84.87]) by omta15.westchester.pa.mail.comcast.net with comcast id GsJP1h0181t3BNj3bsJQ8H; Sat, 06 Aug 2011 04:18:25 +0000 Received: by icarus.home.lan (Postfix, from userid 1000) id 42F48102C36; Fri, 5 Aug 2011 21:18:22 -0700 (PDT) Date: Fri, 5 Aug 2011 21:18:22 -0700 From: Jeremy Chadwick To: Steven Hartland Message-ID: <20110806041822.GA11439@icarus.home.lan> References: <20110728012437.GA23430@icarus.home.lan> <20110728103234.GA33275@icarus.home.lan> <20110728145917.GA37805@icarus.home.lan> <2A07CD8AE6AE49A5BAED59A7E547D1F9@multiplay.co.uk> <2D117F9F212A4CCBA6B7F51E8705BDB7@multiplay.co.uk> <20110805033001.GA47366@icarus.home.lan> <20110805044725.GA48395@icarus.home.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-fs@FreeBSD.ORG Subject: Re: Questions about erasing an ssd to restore performance under FreeBSD X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 06 Aug 2011 04:18:26 -0000 On Sat, Aug 06, 2011 at 01:44:02AM +0100, Steven Hartland wrote: > ----- Original Message ----- From: "Jeremy Chadwick" > > >I've cleaned up the patch (removed the half-written usage stuff) and > >made it available. > > > >http://jdc.parodius.com/freebsd/camcontrol_ata_security/ > > > >If this is committed to base the #define ATA_SECURITY_* entries should > >be moved into include/sys/ata.h. > > > >Steve, if you want to put up your patch somewhere I can review it, but > >an official review from someone more familiar with CAM (e.g. mav@) would > >be best. > > > >I'm also not sure how you implemented all the features, > >UI-wise (command-line-argument-wise). This is what I came up with, from > >my internal docs, with comparative syntax in Linux hdparm: > > > >NOTE: Should try to avoid using -C, -E, -n, -t, -u, or -v > > > >camcontrol security -U -p PWD == unlock (--security-unlock PWD) > >camcontrol security -S -p PWD == set password (--security-set-pass PWD) > >camcontrol security -D -p PWD == disable (--security-disable PWD) > >camcontrol security -X -p PWD == erase (--security-erase PWD) > >camcontrol security -Z -p PWD == enhanced erase (--security-erase-enhanced PWD) > >camcontrol security -i TYPE ... == {user,master} (--user-master USER) > > Yer I couldn't stand using meaningless short options so added long arg support. > > The current version of my patch can be found here:- > http://blog.multiplay.co.uk/dropzone/freebsd/ata_security_cam.patch > > If you can find some time to review it Jeremy that would great. I think > its all pretty straight forward, the only confusing part of the diff is > that I split ataidentify into 3 pieces, ataidentify and the helpers > ata_do_idenfity and ata_cam_send to avoid swathes of code duplication. I've spent about an hour going over the patch, and I'm far from done. Comments in passing or things of concern at this point, some of which have nothing to do directly with your work per se. I have not tested your patch either, just for the record (I do have a spare SSD set up solely for this purpose). 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 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". 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; } Linux hdparm's code says "if revision is 0xffff, then make it zero. If revision isn't 0xffff then don't worry about it. Increment revision." data[] is the 512-byte payload: if (revcode == 0xffff) { revcode = 0; } revcode += 1; data[34] = revcode; data[35] = revcode >> 8; I spent some time looking at ATA8-ACS2 despite spending hours in it last week. All it states is that revision being 0x0000 or 0xffff means the master password feature isn't supported. So that brings into question two things: i) What purpose the 0 != --pwd.revision comparison serves, ii) Why Linux chooses to increment revision rather than set it to something statically. 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. 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...? 4) Do we really need atasecurity_print_time()? (Yes I understand why it exists per se, since it's called twice, but still...) 5) This can be shortened to use ^= instead: pwd.ctrl = pwd.ctrl ^ ATA_SECURITY_PASSWORD_MASTER; 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?") 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). 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()). 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. 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. > 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.)). 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'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. -- | Jeremy Chadwick jdc at parodius.com | | Parodius Networking http://www.parodius.com/ | | UNIX Systems Administrator Mountain View, CA, US | | Making life hard for others since 1977. PGP 4BD6C0CB |