[Drbd-dev] drbd-svn can't handle modular hmac, and crashes if hash can't be found

[Drbd-dev] drbd-svn can't handle modular hmac, and crashes if hash can't be found

[Mark Glines]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

Hi,

A few minor issues.

1.  CONFIG_CRYPTO_HMAC can be defined as a module; the drbd code 
currently requires it to be built into the kernel, apparently for no 
good reason.

2.  If the hash algorithm itself isn't available (like sha1.ko wasn't 
loaded yet), crypto_alloc_hash() returns -ENOENT.  It does not return 
NULL, which is what the drbd code currently checks for.  This leads to 
crashes later.

3.  Once #2 is fixed, the user tools try to report the error, and the 
error message contains a typo, an extra ")".  "Failure: (120) The 
'cram-hmac-alg' you specified is not known in )the kernel."  It also 
doesn't tell the user what to do to fix it.

The attached patch fixes the above issues, and applies cleanly to your 
current svn.  Please review and apply.

Mark

P.S.  Since drbd no longer requires any exported symbols from the 
hmac/hash module, is there any reason to check CONFIG_CRYPTO_HMAC at 
all?  A simpler check on CONFIG_CRYPTO to make sure the basic API exists 
should be all you need...

[-- Attachment #2: drbd-hmac-fixes.diff --]
[-- Type: text/plain, Size: 1728 bytes --]

Index: drbd/drbd_receiver.c
===================================================================
--- drbd/drbd_receiver.c	(revision 2687)
+++ drbd/drbd_receiver.c	(working copy)
@@ -2894,7 +2894,7 @@
 	return 0;
 }
 
-#ifndef CONFIG_CRYPTO_HMAC
+#if !defined(CONFIG_CRYPTO_HMAC) && !defined(CONFIG_CRYPTO_HMAC_MODULE)
 STATIC int drbd_do_auth(drbd_dev *mdev)
 {
 	ERR( "This kernel was build without CONFIG_CRYPTO_HMAC.\n");
Index: drbd/drbd_nl.c
===================================================================
--- drbd/drbd_nl.c	(revision 2687)
+++ drbd/drbd_nl.c	(working copy)
@@ -1052,7 +1052,8 @@
 	if( new_conf->cram_hmac_alg[0] != 0) {
 		snprintf(hmac_name,HMAC_NAME_L,"hmac(%s)",new_conf->cram_hmac_alg);
 		tfm = crypto_alloc_hash(hmac_name, 0, CRYPTO_ALG_ASYNC);
-		if (tfm == NULL) {
+		if (IS_ERR(tfm)) {
+			tfm = NULL;
 			retcode=CRAMAlgNotAvail;
 			goto fail;
 		}
Index: user/drbdsetup.c
===================================================================
--- user/drbdsetup.c	(revision 2687)
+++ user/drbdsetup.c	(working copy)
@@ -359,8 +359,8 @@
 	EM(MDIOError) = "IO error(s) orruced during initial access to meta-data.\n",
 	EM(MDInvalid) = "No valid meta-data signature found.\n)"
 	"Use 'drbdadm create-md res' to initialize meta-data area.\n",
-	EM(CRAMAlgNotAvail) = "The 'cram-hmac-alg' you specified is not known in )"
-	"the kernel.\n",
+	EM(CRAMAlgNotAvail) = "The 'cram-hmac-alg' you specified is not known in "
+	"the kernel. (maybe you need to modprobe it, or modprobe hmac?)\n",
 	EM(CRAMAlgNotDigest) = "The 'cram-hmac-alg' you specified is not a digest.",
 	EM(KMallocFailed) = "kmalloc() failed. Out of memory?",
 	EM(DiscardNotAllowed) = "--discard-my-data not allowed when primary.",

Re: [Drbd-dev] drbd-svn can't handle modular hmac, and crashes if hash can't be found

[Lars Ellenberg]

/ 2007-01-14 09:18:44 -0800
\ Mark Glines:
> Hi,
> 
> A few minor issues.
> 
> 1.  CONFIG_CRYPTO_HMAC can be defined as a module; the drbd code currently requires it to be built into the kernel, 
> apparently for no good reason.
> 
> 2.  If the hash algorithm itself isn't available (like sha1.ko wasn't loaded yet), crypto_alloc_hash() returns 
> -ENOENT.  It does not return NULL, which is what the drbd code currently checks for.  This leads to crashes later.

right... we know 't was unfinished code,
but checked in anyways, to get it going.

> 3.  Once #2 is fixed, the user tools try to report the error, and the error message contains a typo, an extra ")".  
> "Failure: (120) The 'cram-hmac-alg' you specified is not known in )the kernel."  It also doesn't tell the user what 
> to do to fix it.
> 
> The attached patch fixes the above issues, and applies cleanly to your current svn.  Please review and apply.

thanks, will do.

> P.S.  Since drbd no longer requires any exported symbols from the hmac/hash module, is there any reason to check 
> CONFIG_CRYPTO_HMAC at all?  A simpler check on CONFIG_CRYPTO to make sure the basic API exists should be all you 
> need...

we build against older kernels as well,
and as long as debian, suse, redhat & co have their
vendor kernels the way they are, this probably needs to stay this way.
we should be able to relax it for builds against newer kernels, though.

-- 
: Lars Ellenberg                            Tel +43-1-8178292-55 :
: LINBIT Information Technologies GmbH      Fax +43-1-8178292-82 :
: Vivenotgasse 48, A-1120 Vienna/Europe    http://www.linbit.com :

Re: [Drbd-dev] drbd-svn can't handle modular hmac, and crashes if hash can't be found

[Mark Glines]

Lars Ellenberg wrote:
>> 2.  If the hash algorithm itself isn't available (like sha1.ko wasn't loaded yet), crypto_alloc_hash() returns 
>> -ENOENT.  It does not return NULL, which is what the drbd code currently checks for.  This leads to crashes later.
> 
> right... we know 't was unfinished code,
> but checked in anyways, to get it going.

FYI, it's working here on 2.6.20-rc4 (PPC) with CONFIG_CRYPTO_HMAC and 
CONFIG_CRYPTO_SHA1 both configured as modules.  So now I'm watching my 
new cluster sync up for the first time:

drbd0: Writing meta data super block now.
drbd0: conn( WFConnection -> WFReportParams )
drbd0: Handshake successful: DRBD Network Protocol version 85
drbd0: Peer authenticated usind 20 bytes of 'sha1' HMAC
drbd0: peer( Unknown -> Secondary ) conn( WFReportParams -> WFBitMapS ) 
pdsk( DUnknown -> Inconsistent )
drbd0: Writing meta data super block now.
drbd0: conn( WFBitMapS -> SyncSource )
drbd0: Began resync as SyncSource (will sync 9775216 KB [2443804 bits set]).


>> P.S.  Since drbd no longer requires any exported symbols from the hmac/hash module, is there any reason to check 
>> CONFIG_CRYPTO_HMAC at all?  A simpler check on CONFIG_CRYPTO to make sure the basic API exists should be all you 
>> need...
> 
> we build against older kernels as well,
> and as long as debian, suse, redhat & co have their
> vendor kernels the way they are, this probably needs to stay this way.
> we should be able to relax it for builds against newer kernels, though.

Makes sense.  Thanks again!

Mark