Re: [PATCH CIFS] use CryptoAPI MD4/MD5

[Francois Romieu <romieu@fr.zoreil.com>]

dean gaudet <dean-list-linux-kernel@arctic.org> :
[...]
> this really sounds like two steps backwards and one step forward.
> 
> this CIFS patch alone replaces 89 lines with 250 lines of code!  the new

It could factor this sequence for example:
       xxx.page = virt_to_page(key);
       xxx.offset = offset_in_page(key);
       xxx.length = foo;
(appears 19 times in the patch)

Btw, the patch add things like this (fs/cifs/connect.c::cifs_mount):

@@ -917,7 +918,18 @@
                        sprintf(pSesInfo->serverName, "%u.%u.%u.%u",
                                NIPQUAD(sin_server.sin_addr.s_addr));
                }
-
+               if (!rc) {
+                       pSesInfo->md5_tfm = crypto_alloc_tfm("md5", 0);
+                       if (unlikely(pSesInfo->md5_tfm == NULL)) {
+                               printk(KERN_WARNING "failed to load transform
for md5\n");
+                               rc = -ENOMEM;
+                       }
+                       pSesInfo->md4_tfm = crypto_alloc_tfm("md4", 0);
+                       if (unlikely(pSesInfo->md4_tfm == NULL)) {
+                               printk(KERN_WARNING "failed to load transform
for md4\n");
+                               rc = -ENOMEM;
+                       }
+               }
                if (!rc){
                        if (volume_info.password)
                                strncpy(pSesInfo->password_with_pad,
@@ -999,6 +1011,14 @@

 /* on error free sesinfo and tcon struct if needed */
        if (rc) {
+               if (pSesInfo->md5_tfm != NULL) {
+                       crypto_free_tfm(pSesInfo->md5_tfm);
+                       pSesInfo->md5_tfm = NULL;
+               }
+               if (pSesInfo->md4_tfm != NULL) {
+                       crypto_free_tfm(pSesInfo->md4_tfm);
+                       pSesInfo->md4_tfm = NULL;
+               }
                           /* If find_unc succeeded then rc == 0 so we can not
end */
                if (tcon)  /* up here accidently freeing someone elses tcon
struct */
                        tconInfoFree(tcon);
@@ -2422,6 +2442,14 @@

The code doesn't remember what failed and must do extra checks. This adds
lines of code rather fast. Not a surprise in a function with a pile of 
"if (rc)" and multiple exit paths.

> code does not look anywhere near as readable as the original.  but perhaps

fs/cifs/connect.c::CIFSNTLMSSPNegotiateSessSetup):
[...]
                                                        ses-> 
                                                            serverDomain[1
                                                                         +
                                                                         (2
                                                                          *
                                                                          len)]
                                                            = 0;

              Imho
CryptoAPI
                      is
not
         the                                   biggest
                 problem
               here

--
Ueimor