Date: Mon, 7 Oct 2013 18:24:51 -0700 From: John-Mark Gurney <jmg@funkthat.com> To: Gleb Kurtsou <gleb.kurtsou@gmail.com> Cc: Gleb Kurtsou <gleb@freebsd.org>, "freebsd-current@freebsd.org" <freebsd-current@freebsd.org>, "delphij@freebsd.org" <delphij@freebsd.org>, Kris Moore <kris@pcbsd.org> Subject: Re: Committing PEFS to CURRENT Message-ID: <20131008012451.GE56872@funkthat.com> In-Reply-To: <CADDB7yFsCfBpVvV4ew866ANr_=CpjENmjen2Oakm5wbmwZ0cQQ@mail.gmail.com> References: <20131007163111.GB1590@reks.swifttest.com> <20131007231734.GY56872@funkthat.com> <CADDB7yFR-s2PExPCru2H9TM%2Bgfi6LoV0XygPGR=NH%2Bxir=bBOw@mail.gmail.com> <20131008002314.GA56872@funkthat.com> <CADDB7yFsCfBpVvV4ew866ANr_=CpjENmjen2Oakm5wbmwZ0cQQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Gleb Kurtsou wrote this message on Mon, Oct 07, 2013 at 17:41 -0700: > On Mon, Oct 7, 2013 at 5:23 PM, John-Mark Gurney <jmg@funkthat.com> wrote: > > Gleb Kurtsou wrote this message on Mon, Oct 07, 2013 at 16:47 -0700: > >> On Mon, Oct 7, 2013 at 4:17 PM, John-Mark Gurney <jmg@funkthat.com> wrote: > >> > Gleb Kurtsou wrote this message on Mon, Oct 07, 2013 at 09:31 -0700: > >> >> Patch is available here: > >> >> https://github.com/glk/freebsd-head/commit/b4d2c4a5f42f88fdd07cb75feba3467e4d4c043c.patch > >> > > >> > Is there a reason you are writing your own AES-NI implementation instead > >> > of using the OpenCrypto framework? > >> > >> It reuses the same AES-NI implementation used by opencrypto, > >> but code doesn't use opencrypto directly. Main limitation in opencrypto is > >> that is incomplete implementation AES-XTS -- it doesn't implement ciphertext > >> stealing. opencrypto contexts seemed to be too much overhead list time I > > > > I remember noticing that when I was working on it.. but as there are > > different modes of packing/padding, I decided not to do anything with > > that... > > > > I also don't like your lack of comments arround xts_lastblock and about > > why it is accessing dst - 1... To me, you should pass in the previous > > block as an arg to xts_lastblock instead of doing dst[-1]... You did > > comment what you're doing (m - 1), but not why it is safe to do that... > > There is no comment that you're implementing ciphertext stealing w/ the > > function which makes it even harder to understand that you'd going it > > properly... > > The code comes from University of Tsukuba. The function is internal to > the module and is safe to use that way if you look at pefs_xts_block_encrypt > and pefs_xts_block_decrypt. That doesn't make it exempt from good coding style or modification to be easier to review... > > It wouldn't be hard to add ciphertext stealing to the opencrypto > > implementation if that is really all that is missing... but.. > > > >> looked at them especially in the case of multiple keys per file system in PEFS. > >> AES-NI interface is not designed to be used outside of opencrypto thus > >> some minor copy-past. > > > > We have discovered that by the "minor" copy/paste we now have an > > inferior implementation of AES-XTS... If it performed similar to the > > one before it, it is over 10x slower than the one that I committed.. > > > >> > I updated the kernel's AES-NI implementation to have a very fast > >> > AES-XTS... Upon looking at your implementation, you have a very > >> > slow implementation as you do not pipeline AES-XTS at all... Please > >> > switch to using the opencrypto version.. You'll then be able to make > >> > use of any accelerators that other platforms may have... > >> > >> I was looking into incorporating AES_XTS pipeline changes in PEFS. > >> I'd like to avoid switching to opencrypto at this point. But pipelining is > >> doable without opencrypto and copying code around. > > > > I really don't like the idea of adding yet anothe AES-XTS implementation > > to our tree (especially considering how bad both the previous one and > > this one is)... Even if you do bring over the pipeline changes... > > It'll be yet another copy of code to maintain and port performance > > improvements too... > > > > We could always refactor the AES-NI code to make it more usable outside > > the opencrypto framework as a stepping stone to possibly using pjd's > > improved opencrypto framework... > > Refactoring AES-NI would be ideal, it would also be great to extract AES-XTS > implementation and make it usable outside of opencrypto adding ciphertext > stealing. As for making it usable outside of opencrypto, I'd prefer that we figure out if we should use pjd's improvements, or if we really want to make AES-NI a first class citizen in the kernel, and if the later, figuring out a proper API for it... I'm nervous that we are rushing these changes into the tree... I do like the idea of getting PEFS into the kernel, but it doesn't feel like it's been properly reviewed/integrated yet... > > But copy/pasting just because we don't want to do a bit more work isn't > > good justification... > > It's not that I "don't want to do a bit more work" I never said that, > it's rather > about avoiding changes after KBI freeze. But will the work get done to clean it up after the freeze is over? What happens if it doesn't, will it get removed before 10.1 or will we have to live w/ the code? > >> > Are there plans to add authentication to this scheme? See that as a > >> > todo, but w/o authentication, you can't store anything reliably on it.. > >> > And w/ XTS, the attacker can take pot shots at your file in 16 byte > >> > chuncks... > >> > >> I have data authentication prototype. It's too far from being complete, > >> and I keep working on it as time permits. There are a few more ideas > >> I'd like to work on while redesigning the file system. > >> > >> Patch also includes hmac and pkcs5v2 implementations which in fact > >> were generic versions to go under sys/crypto until yesterday. > >> Considering we are late in code freeze already I've merged them > >> into PEFS not to touch geli code with the patch. > > > > Can't you keep them named the same under sys/crypto and just link w/ them > > as necessary to prevent repo churn when you finally integrate them into > > sys/crypto? That seems better than moving them arround, though I guess > > w/ svn, it isn't as big of a deal... Someone w/ a repo hat on should > > chime in here... > > It won't be possible at least because of pkcs5v2_genkey() (name collision) > being defined internally in geli and pkcs5v2 using hmac from geli. Moving > them to sys/crypto in 11-CURRENT is a minor issue IMHO. I was pushing > those changes to HEAD years ago, but they got stuck somewhere in > review process. ok... > >> > The only reason I'm running zfs on geli w/o authentication is that I'm > >> > using a 256bit checksum, so the chances of someone modifing two blocks > >> > to fool zfs into decrypting the correct new checksum value for their > >> > modified block is very small... In short, I'm trusting zfs to do the > >> > authentication for me... As long as we have a plan forward for what needs to be done and what will be done, I'm fine... -- John-Mark Gurney Voice: +1 415 225 5579 "All that I will do, has been done, All that I have, has not."
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131008012451.GE56872>