On 2013-06-03 23:46:44, Will Morrison wrote: > On 13-06-03 09:51 PM, Tyler Hicks wrote: > > > To start things off... when you have a side note, such as your > > paragraph above mentioning that this is your first kernel patch, > > you don't want something like that to end up in the commit log > > history forever. A good place for notes like that is below the --- > > line and above the diff command. Most patch merging tools should > > ignore that content and keep it from being recorded forever in the > > commit history. (I know that you didn't intend for this patch to be > > upstreamed at this time, but I thought I'd mention this tip.) > > Didn't know that, thanks for the tip. :) > > > I'll assume that this means that you're not using git. If you plan > > to continue doing kernel development (or really any open source > > development) after this feature, I'd recommend that you work on > > acquiring basic git skills. You previously asked what can help > > speed up your development workflow and understanding git is > > certainly something that will help. > > We are actually using git for development. I used diff to generate the > patch because I was following Documentation/SubmittingPatches, which > gives diff commands. > > What should we be using to generate the patches? git diff? git format-patch will generate the patches and save them as files git send-email will email those patches out. You can also use git format-patch directly and skip the git format-patch step if you'd like. > > > In fact, this patch won't apply for me using `git am -is > > /path/to/mbox`. I'm not immediately sure why, but I do see some > > extra '-' characters in the areas that git complains about. > > > > It might be because you GPG-signed the email. GPG signing patches > > would seem like a good thing to do, but > > Documentation/email-clients.txt claims that it causes some problems > > and recommends that you don't sign patches. > > I can disable GPG signing for patches easily enough, or switch to > using PGP/MIME. Would one or the other be preferable? I don't know if > git can deal with PGP/MIME emails. I'm not sure about PGP/MIME. I know that unsigned works perfectly so that's probably the easiest option. > > > Running the following search and replace in vim: > > > > :%s/^- -/-/g > > > > and stripping off the GPG header/footer, I'm kind of able to get > > the patch to apply. There's offset and fuzz issues, so git am won't > > apply it (but patch will, of course). Which kernel tree did you > > develop the patch against? > > will@Aldor ~/FYDP/ecryptfs_base $ git log -1 > commit f6161aa153581da4a3867a2d1a7caf4be19b6ec9 > Author: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> > Date: Sun Mar 10 16:54:19 2013 -0700 > > Linux 3.9-rc2 So a little bit behind. Typically, you'd want to rebase and test your patches at least against current mainline before you send them out. Even better would be to do it against my next branch (http://git.kernel.org/cgit/linux/kernel/git/tyhicks/ecryptfs.git/log/?h=next), which contains the latest eCryptfs patches, but mainline will probably offer a more stable kernel for you to work against. I'm really not picky though. eCryptfs development moves slow enough that you will normally be fine developing against a slightly older codebase. If there's a big conflict, I'll just ask you to fix it. I'll handle small conflicts myself. > > >> + strncpy(crypt_stat->cipher_mode, + > >> crypt_stat->mount_crypt_stat->global_default_cipher_mode_name, + > >> ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE); > > > > This is a problem. eCryptfs files are atomic in that they contain > > all of the information needed to decrypt the file (except for the > > FEKEK) in the header. So a single eCryptfs mount may contain files > > encrypted by AES-128, AES-256, blowfish, the FEK can be wrapped > > with a public key, multiple FEKEKs can be used, etc., and eCryptfs > > can seamlessly handle it because of the various metadata stored in > > the file header. > > > > At first glance, it doesn't look like RFC2440 defines a field for > > different cipher modes. eCryptfs has always just made the > > assumption that it should use its tweaked CBC mode and that has > > always been sufficient. > > > > We will need a place to store the cipher mode in the header and > > that field will need to be read in somewhere around here. > > > > Also, when writing out the header information, we'll need to store > > the cipher mode. > > This is something we've been discussing amongst ourselves, and were > planning on addressing that in another patch. This was mainly a > stop-gap until then. However, since I don't see anything based on a > quick read through the RFCs you mentioned, I'll send an email sometime > tomorrow regarding this. Sounds good. > > >> + const char *mode_white_list[] = {"cbc", "ecb", "gcm"}; > > > > When you submit a patch, it should be able to stand on its own. > > This patch doesn't stand on its own because ecb and gcm aren't > > supported but this patch happily accepts them. This white list > > should only contain "cbc" at this point. > > > > The idea you had with testing with ECB was a good idea, but it > > isn't something that we'd want to ever support in eCryptfs since > > our default is more secure. So the "ecb" element should of just > > been a local change that you made on top of this patch, only for > > testing. > > Will have that removed for actual patch submission. > > >> + cipher_mode_name_dst = + mount_crypt_stat-> + > >> global_default_cipher_mode_name; > > > > Yuck. I don't know if this is better or worse than breaking the 80 > > char rule. eCryptfs uses variable names that are too long in most > > cases. (This isn't something that you need to worry about) > > Are there specific exceptions to the 80 char rule, or is this an area > where it's down to experience and judgment? Printed strings are one example for breaking the 80 char rule (eCryptfs wraps them in most cases, though). But, it mostly comes down to experience/judgement. It is probably best if you stay within the 80 char limit in most situations. If I have a problem with some line wrapping, I'll fix it up while merging the patch. > > >> + BUG_ON(cipher_mode_name_len >= + > >> ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE); > > > > I see that you copied and pasted this logic from the > > !cipher_name_set conditional. So it isn't your fault, but this is > > terrible. > > > > This is something that should be caught at build time rather than > > at runtime (why force this BUG() on our users?). These BUG_ON()'s > > should probably be replaced with a BUILD_BUG_ON(). > > I'm not too familiar with kernel macros, but my assumption based on a > quick google search is that this is something like an assert? I haven't actually used BUILD_BUG* before. I stumbled upon it after not liking that runtime BUG_ON(). However, the comments of BUILD_BUG_ON() are pretty clear: /** * BUILD_BUG_ON - break compile if a condition is true. * @condition: the condition which the compiler should know is false. * * If you have some code which relies on certain constants being equal, or * some other compile-time-evaluated condition, you should use BUILD_BUG_ON to * detect if someone changes it. You'll have to give it a try and make sure it works as you expect. Tyler > > > This was a nice submission for your first shot at it. The biggest > > problem to fix will be figuring out where to store the mode in the > > header. Take a look through RFC2440 (and 4880 and 5581) to see if > > there's already a field we can use for this. If not, we'll have to > > give it some more thought. I don't want to rely on the user > > specifying the correct ecryptfs_cipher_mode mount option. > > > > Thanks again for working on this! > > > > Tyler > > > Thanks for being helpful and responsive. :)
Attachment:
signature.asc
Description: Digital signature
