[stgt] [PATCH] SEM Key Clash with Open-iSCSI initiator and FHS correction for ipc socket

[Roland Friedwagner]

Am Montag 07 März 2011 schrieb Or Gerlitz:
> Roland Friedwagner wrote:
> >  - Fix logging semaphore key clash between iscsd and tgtd
>
> I tend to think we have that/similar clash also between multiple tgtd
> instances, e.g running two instances and using strace, I can see that
> both did "semget(0x54475444, 1, IPC_CREAT|0666)" is that also
> something we need to address?
>
> Or.
>

I think you are right!


My suggested patch did create a ipc semaphore keyid based on
the managment socket file by calling ftok(3).
To avoid to have the mgmt socket path in mgmt.c and log.c 
I made it extern in mgmt.c 
(for the symbol uniqueness change the name to mgmt_path).

Also log a warning if the semkey already exits (ipcs -m), but
continue to initialize tgtd.
(I think have tgtd running to serve requests has priority to
some possible logging clash with other instance.)

Suggestions/Improvements?

---------------------------------%<----------------------------------------
diff -Nurp tgt-1.0.14.orig/usr/log.c tgt-1.0.14/usr/log.c
--- tgt-1.0.14.orig/usr/log.c	2011-03-01 12:53:45.000000000 +0100
+++ tgt-1.0.14/usr/log.c	2011-03-08 12:09:16.000000000 +0100
@@ -33,7 +33,6 @@
 
 #include "log.h"
 
-#define SEMKEY	0xA7L
 #define LOGDBG 0
 
 #if LOGDBG
@@ -50,6 +49,8 @@ static pid_t pid;
 static int logarea_init (int size)
 {
 	int shmid;
+	extern char mgmt_path[];
+	key_t semkey;
 
 	logdbg(stderr,"enter logarea_init\n");
 
@@ -109,13 +110,23 @@ static int logarea_init (int size)
 
 	shmctl(shmid, IPC_RMID, NULL);
 
-	if ((la->semid = semget(SEMKEY, 1, 0666 | IPC_CREAT)) < 0) {
+	if ((semkey = ftok(mgmt_path, 'a')) < 0) {
+		syslog(LOG_ERR, "semkey failed %d", errno);
+		return 1;
+	}
+
+        if ((semget(semkey, 0, IPC_EXCL)) > 0) {
+		/* Semkey may exists after SIGKILL tgtd */
+		syslog(LOG_WARNING, "semkey 0x%x already exists", semkey);
+	}
+	if ((la->semid = semget(semkey, 1, 0666 | IPC_CREAT)) < 0) {
 		syslog(LOG_ERR, "semget failed %d", errno);
 		shmdt(la->buff);
 		shmdt(la->start);
 		shmdt(la);
 		return 1;
 	}
+	syslog(LOG_DAEMON, "semkey 0x%x", semkey);
 
 	la->semarg.val=1;
 	if (semctl(la->semid, 0, SETVAL, la->semarg) < 0) {
diff -Nurp tgt-1.0.14.orig/usr/mgmt.c tgt-1.0.14/usr/mgmt.c
--- tgt-1.0.14.orig/usr/mgmt.c	2011-03-01 12:53:45.000000000 +0100
+++ tgt-1.0.14/usr/mgmt.c	2011-03-08 11:59:04.000000000 +0100
@@ -59,6 +59,7 @@ struct mgmt_task {
 };
 
 static int ipc_fd;
+char mgmt_path[256];
 
 static void set_show_results(struct tgtadm_rsp *rsp, int *err)
 {
@@ -567,7 +568,7 @@ int ipc_init(void)
 	extern short control_port;
 	int fd, err;
 	struct sockaddr_un addr;
-	char path[256];
+	extern char mgmt_path[];
 
 	fd = socket(AF_LOCAL, SOCK_STREAM, 0);
 	if (fd < 0) {
@@ -575,11 +576,11 @@ int ipc_init(void)
 		return -1;
 	}
 
-	sprintf(path, "%s.%d", TGT_IPC_NAMESPACE, control_port);
-	unlink(path);
+	sprintf(mgmt_path, "%s.%d", TGT_IPC_NAMESPACE, control_port);
+	unlink(mgmt_path);
 	memset(&addr, 0, sizeof(addr));
 	addr.sun_family = AF_LOCAL;
-	strncpy(addr.sun_path, path, sizeof(addr.sun_path));
+	strncpy(addr.sun_path, mgmt_path, sizeof(addr.sun_path));
 
 	err = bind(fd, (struct sockaddr *) &addr, sizeof(addr));
 	if (err) {
diff -Nurp tgt-1.0.14.orig/usr/tgtadm.c tgt-1.0.14/usr/tgtadm.c
--- tgt-1.0.14.orig/usr/tgtadm.c	2011-03-01 12:53:45.000000000 +0100
+++ tgt-1.0.14/usr/tgtadm.c	2011-03-08 11:59:43.000000000 +0100
@@ -190,7 +190,7 @@ static int ipc_mgmt_connect(int *fd)
 {
 	int err;
 	struct sockaddr_un addr;
-	char path[256];
+	char mgmt_path[256];
 
 	*fd = socket(AF_LOCAL, SOCK_STREAM, 0);
 	if (*fd < 0) {
@@ -200,8 +200,8 @@ static int ipc_mgmt_connect(int *fd)
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sun_family = AF_LOCAL;
-	sprintf(path, "%s.%d", TGT_IPC_NAMESPACE, control_port);
-	strncpy(addr.sun_path, path, sizeof(addr.sun_path));
+	sprintf(mgmt_path, "%s.%d", TGT_IPC_NAMESPACE, control_port);
+	strncpy(addr.sun_path, mgmt_path, sizeof(addr.sun_path));
 
 	err = connect(*fd, (struct sockaddr *) &addr, sizeof(addr));
 	if (err < 0)
---------------------------------%<----------------------------------------

Kind Regards,
Roland

-- 
Roland.Friedwagner at wu.ac.at            Phone: +43 1 31336 5377
IT Services - WU (Vienna University of Economics and Business) 
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html