Questions about erasing an ssd to restore performance under
FreeBSD
Jeremy Chadwick
freebsd at jdc.parodius.com
Sat Aug 6 04:18:26 UTC 2011
On Sat, Aug 06, 2011 at 01:44:02AM +0100, Steven Hartland wrote:
> ----- Original Message ----- From: "Jeremy Chadwick"
> <freebsd at jdc.parodius.com>
> >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 |
More information about the freebsd-fs
mailing list