FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 12-21-2011, 07:17 AM
Hannes Reinecke
 
Default multipath: fix scsi async tur checker corruption

On 12/21/2011 12:01 AM, Benjamin Marzinski wrote:
> Since the tur checker runs asynchronously in its own thread, there is nothing
> that keeps a path from being orphaned or deleted before the tur thread has
> finished. When this happenes the checker struct gets deleted. However, the tur
> thread might still we writing to that memory. This can lead to memory
> corruption. This patch adds all of the necessary data to the checker context,
> and makes the tur thread only use that. This way, if the checker is deleted
> while the thread is still using the context, the thread will clean up the
> context itself.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/checkers/tur.c | 85 +++++++++++++++++++++++++++++++++-----------
> libmultipath/discovery.c | 1
> 2 files changed, 65 insertions(+), 21 deletions(-)
>
> Index: multipath-tools-111219/libmultipath/checkers/tur.c
> ================================================== =================
> --- multipath-tools-111219.orig/libmultipath/checkers/tur.c
> +++ multipath-tools-111219/libmultipath/checkers/tur.c
> @@ -35,10 +35,15 @@ struct tur_checker_context {
> dev_t devt;
> int state;
> int running;
> - time_t timeout;
> + int fd;
> + unsigned int timeout;
> + time_t time;
> pthread_t thread;
> pthread_mutex_t lock;
> pthread_cond_t active;
> + pthread_spinlock_t hldr_lock;
> + int holders;
> + char message[CHECKER_MSG_LEN];
> };
>
> #define TUR_DEVT(c) major((c)->devt), minor((c)->devt)
> @@ -53,28 +58,49 @@ int libcheck_init (struct checker * c)
> memset(ct, 0, sizeof(struct tur_checker_context));
>
> ct->state = PATH_UNCHECKED;
> + ct->fd = -1;
> + ct->holders = 1;
> pthread_cond_init(&ct->active, NULL);
> pthread_mutex_init(&ct->lock, NULL);
> + pthread_spin_init(&ct->hldr_lock, PTHREAD_PROCESS_PRIVATE);
> c->context = ct;
>
> return 0;
> }
>
> +void cleanup_context(struct tur_checker_context *ct)
> +{
> + pthread_mutex_destroy(&ct->lock);
> + pthread_cond_destroy(&ct->active);
> + pthread_spin_destroy(&ct->hldr_lock);
> + free(ct);
> +}
> +
> void libcheck_free (struct checker * c)
> {
> if (c->context) {
> struct tur_checker_context *ct = c->context;
> + int holders;
> + pthread_t thread;
>
> - pthread_mutex_destroy(&ct->lock);
> - pthread_cond_destroy(&ct->active);
> - free(c->context);
> + pthread_spin_lock(&ct->hldr_lock);
> + ct->holders--;
> + holders = ct->holders;
> + thread = ct->thread;
> + pthread_spin_unlock(&ct->hldr_lock);
> + if (holders)
> + pthread_cancel(thread);
> + else
> + cleanup_context(ct);
> c->context = NULL;
> }
> return;
> }
>
> +#define TUR_MSG(msg, fmt, args...) snprintf(msg, CHECKER_MSG_LEN, fmt, ##args);
> +
> int
> -tur_check (struct checker * c)
> +tur_check(int fd, unsigned int timeout, char *msg)
> {
> struct sg_io_hdr io_hdr;
> unsigned char turCmdBlk[TUR_CMD_LEN] = { 0x00, 0, 0, 0, 0, 0 };
> @@ -90,10 +116,10 @@ tur_check (struct checker * c)
> io_hdr.dxfer_direction = SG_DXFER_NONE;
> io_hdr.cmdp = turCmdBlk;
> io_hdr.sbp = sense_buffer;
> - io_hdr.timeout = c->timeout;
> + io_hdr.timeout = timeout;
> io_hdr.pack_id = 0;
> - if (ioctl(c->fd, SG_IO, &io_hdr) < 0) {
> - MSG(c, MSG_TUR_DOWN);
> + if (ioctl(fd, SG_IO, &io_hdr) < 0) {
> + TUR_MSG(msg, MSG_TUR_DOWN);
> return PATH_DOWN;
> }
> if ((io_hdr.status & 0x7e) == 0x18) {
> @@ -101,7 +127,7 @@ tur_check (struct checker * c)
> * SCSI-3 arrays might return
> * reservation conflict on TUR
> */
> - MSG(c, MSG_TUR_UP);
> + TUR_MSG(msg, MSG_TUR_UP);
> return PATH_UP;
> }
> if (io_hdr.info & SG_INFO_OK_MASK) {
> @@ -146,14 +172,14 @@ tur_check (struct checker * c)
> * LOGICAL UNIT NOT ACCESSIBLE,
> * TARGET PORT IN STANDBY STATE
> */
> - MSG(c, MSG_TUR_GHOST);
> + TUR_MSG(msg, MSG_TUR_GHOST);
> return PATH_GHOST;
> }
> }
> - MSG(c, MSG_TUR_DOWN);
> + TUR_MSG(msg, MSG_TUR_DOWN);
> return PATH_DOWN;
> }
> - MSG(c, MSG_TUR_UP);
> + TUR_MSG(msg, MSG_TUR_UP);
> return PATH_UP;
> }
>
> @@ -162,18 +188,25 @@ tur_check (struct checker * c)
>
> void cleanup_func(void *data)
> {
> + int holders;
> struct tur_checker_context *ct = data;
> + pthread_spin_lock(&ct->hldr_lock);
> + ct->holders--;
> + holders = ct->holders;
> ct->thread = 0;
> + pthread_spin_unlock(&ct->hldr_lock);
> + if (!holders)
> + cleanup_context(ct);
> }
>
> void *tur_thread(void *ctx)
> {
> - struct checker *c = ctx;
> - struct tur_checker_context *ct = c->context;
> + struct tur_checker_context *ct = ctx;
> int state;
>
> condlog(3, "%d:%d: tur checker starting up", TUR_DEVT(ct));
>
> + ct->message[0] = '';
> /* This thread can be canceled, so setup clean up */
> tur_thread_cleanup_push(ct)
>
> @@ -182,7 +215,7 @@ void *tur_thread(void *ctx)
> ct->state = PATH_PENDING;
> pthread_mutex_unlock(&ct->lock);
>
> - state = tur_check(c);
> + state = tur_check(ct->fd, ct->timeout, ct->message);
>
> /* TUR checker done */
> pthread_mutex_lock(&ct->lock);
> @@ -213,7 +246,7 @@ void tur_set_async_timeout(struct checke
> struct timeval now;
>
> gettimeofday(&now, NULL);
> - ct->timeout = now.tv_sec + c->timeout;
> + ct->time = now.tv_sec + c->timeout;
> }
>
> int tur_check_async_timeout(struct checker *c)
> @@ -222,7 +255,7 @@ int tur_check_async_timeout(struct check
> struct timeval now;
>
> gettimeofday(&now, NULL);
> - return (now.tv_sec > ct->timeout);
> + return (now.tv_sec > ct->time);
> }
>
> extern int
> @@ -242,7 +275,7 @@ libcheck_check (struct checker * c)
> ct->devt = sb.st_rdev;
>
> if (c->sync)
> - return tur_check(c);
> + return tur_check(c->fd, c->timeout, c->message);
>
> /*
> * Async mode
> @@ -276,6 +309,8 @@ libcheck_check (struct checker * c)
> /* TUR checker done */
> ct->running = 0;
> tur_status = ct->state;
> + strncpy(c->message, ct->message, CHECKER_MSG_LEN);
> + c->message[CHECKER_MSG_LEN - 1] = '';
> }
> pthread_mutex_unlock(&ct->lock);
> } else {
> @@ -284,24 +319,32 @@ libcheck_check (struct checker * c)
> pthread_mutex_unlock(&ct->lock);
> condlog(3, "%d:%d: tur thread not responding, "
> "using sync mode", TUR_DEVT(ct));
> - return tur_check(c);
> + return tur_check(c->fd, c->timeout, c->message);
> }
> /* Start new TUR checker */
> ct->state = PATH_UNCHECKED;
> + ct->fd = c->fd;
> + ct->timeout = c->timeout;
> + pthread_spin_lock(&ct->hldr_lock);
> + ct->holders++;
> + pthread_spin_unlock(&ct->hldr_lock);
> tur_set_async_timeout(c);
> setup_thread_attr(&attr, 32 * 1024, 1);
> - r = pthread_create(&ct->thread, &attr, tur_thread, c);
> + r = pthread_create(&ct->thread, &attr, tur_thread, ct);
> if (r) {
> pthread_mutex_unlock(&ct->lock);
> ct->thread = 0;
> + ct->holders--;
> condlog(3, "%d:%d: failed to start tur thread, using"
> " sync mode", TUR_DEVT(ct));
> - return tur_check(c);
> + return tur_check(c->fd, c->timeout, c->message);
> }
> pthread_attr_destroy(&attr);
> tur_timeout(&tsp);
> r = pthread_cond_timedwait(&ct->active, &ct->lock, &tsp);
> tur_status = ct->state;
> + strncpy(c->message, ct->message,CHECKER_MSG_LEN);
> + c->message[CHECKER_MSG_LEN -1] = '';
> pthread_mutex_unlock(&ct->lock);
> if (ct->thread &&
> (tur_status == PATH_PENDING || tur_status == PATH_UNCHECKED)) {
> Index: multipath-tools-111219/libmultipath/discovery.c
> ================================================== =================
> --- multipath-tools-111219.orig/libmultipath/discovery.c
> +++ multipath-tools-111219/libmultipath/discovery.c
> @@ -826,6 +826,7 @@ get_state (struct path * pp, int daemon)
> }
> checker_set_fd(c, pp->fd);
> if (checker_init(c, pp->mpp?&pp->mpp->mpcontext:NULL)) {
> + memset(c, 0x0, sizeof(struct checker));
> condlog(3, "%s: checker init failed", pp->dev);
> return PATH_UNCHECKED;
> }
>
Hmm. Why do we need ->holders here?
In theory we can have only two states; async thread running or no
thread running. I really cannot imagine a scenario where ->holders
would be > 1.
Or, to stress the point a bit more, any scenario where holders > 1
would most definitely be a bug, as this would indicate that an async
tur thread is running, but we somehow failed to notice this, and
started a new one. Which means we end with two threads doing exactly
the same thing. And which was _exactly_ what we want to avoid with
at startup.
So I guess we can do away with ->holders.
And once ->holders is gone, I doubt it'll make any sense to retain
the spinlock, as we then just have ->thread to worry about.
And this doesn't need to be spinlock protected, as we take care to
synchronize during startup. And during shutdown we simply don't
care, as pthread_cancel() will return an error if the thread is
already done for.

Cheers,

Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-23-2012, 09:05 PM
Benjamin Marzinski
 
Default multipath: fix scsi async tur checker corruption

Since the tur checker runs asynchronously in its own thread, there is nothing
that keeps a path from being orphaned or deleted before the tur thread has
finished. When this happenes the checker struct gets deleted. However, the tur
thread might still we writing to that memory. This can lead to memory
corruption. This patch adds all of the necessary data to the checker context,
and makes the tur thread only use that. This way, if the checker is deleted
while the thread is still using the context, the thread will clean up the
context itself.

Since the context can only be freed when both the thread and the paths checker
structure have stopped needing it, and these can get get finished with the
context asychronously with respect to each other, the context has a holders
counter, protected by a spinlock, to keep track of the users. When the
counter drops to zero, the context gets freed.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/checkers/tur.c | 85 +++++++++++++++++++++++++++++++++-----------
libmultipath/discovery.c | 1
2 files changed, 65 insertions(+), 21 deletions(-)

Index: multipath-tools-120518/libmultipath/checkers/tur.c
================================================== =================
--- multipath-tools-120518.orig/libmultipath/checkers/tur.c
+++ multipath-tools-120518/libmultipath/checkers/tur.c
@@ -35,10 +35,15 @@ struct tur_checker_context {
dev_t devt;
int state;
int running;
- time_t timeout;
+ int fd;
+ unsigned int timeout;
+ time_t time;
pthread_t thread;
pthread_mutex_t lock;
pthread_cond_t active;
+ pthread_spinlock_t hldr_lock;
+ int holders;
+ char message[CHECKER_MSG_LEN];
};

#define TUR_DEVT(c) major((c)->devt), minor((c)->devt)
@@ -53,28 +58,49 @@ int libcheck_init (struct checker * c)
memset(ct, 0, sizeof(struct tur_checker_context));

ct->state = PATH_UNCHECKED;
+ ct->fd = -1;
+ ct->holders = 1;
pthread_cond_init(&ct->active, NULL);
pthread_mutex_init(&ct->lock, NULL);
+ pthread_spin_init(&ct->hldr_lock, PTHREAD_PROCESS_PRIVATE);
c->context = ct;

return 0;
}

+void cleanup_context(struct tur_checker_context *ct)
+{
+ pthread_mutex_destroy(&ct->lock);
+ pthread_cond_destroy(&ct->active);
+ pthread_spin_destroy(&ct->hldr_lock);
+ free(ct);
+}
+
void libcheck_free (struct checker * c)
{
if (c->context) {
struct tur_checker_context *ct = c->context;
+ int holders;
+ pthread_t thread;

- pthread_mutex_destroy(&ct->lock);
- pthread_cond_destroy(&ct->active);
- free(c->context);
+ pthread_spin_lock(&ct->hldr_lock);
+ ct->holders--;
+ holders = ct->holders;
+ thread = ct->thread;
+ pthread_spin_unlock(&ct->hldr_lock);
+ if (holders)
+ pthread_cancel(thread);
+ else
+ cleanup_context(ct);
c->context = NULL;
}
return;
}

+#define TUR_MSG(msg, fmt, args...) snprintf(msg, CHECKER_MSG_LEN, fmt, ##args);
+
int
-tur_check (struct checker * c)
+tur_check(int fd, unsigned int timeout, char *msg)
{
struct sg_io_hdr io_hdr;
unsigned char turCmdBlk[TUR_CMD_LEN] = { 0x00, 0, 0, 0, 0, 0 };
@@ -90,10 +116,10 @@ tur_check (struct checker * c)
io_hdr.dxfer_direction = SG_DXFER_NONE;
io_hdr.cmdp = turCmdBlk;
io_hdr.sbp = sense_buffer;
- io_hdr.timeout = c->timeout;
+ io_hdr.timeout = timeout;
io_hdr.pack_id = 0;
- if (ioctl(c->fd, SG_IO, &io_hdr) < 0) {
- MSG(c, MSG_TUR_DOWN);
+ if (ioctl(fd, SG_IO, &io_hdr) < 0) {
+ TUR_MSG(msg, MSG_TUR_DOWN);
return PATH_DOWN;
}
if ((io_hdr.status & 0x7e) == 0x18) {
@@ -101,7 +127,7 @@ tur_check (struct checker * c)
* SCSI-3 arrays might return
* reservation conflict on TUR
*/
- MSG(c, MSG_TUR_UP);
+ TUR_MSG(msg, MSG_TUR_UP);
return PATH_UP;
}
if (io_hdr.info & SG_INFO_OK_MASK) {
@@ -146,14 +172,14 @@ tur_check (struct checker * c)
* LOGICAL UNIT NOT ACCESSIBLE,
* TARGET PORT IN STANDBY STATE
*/
- MSG(c, MSG_TUR_GHOST);
+ TUR_MSG(msg, MSG_TUR_GHOST);
return PATH_GHOST;
}
}
- MSG(c, MSG_TUR_DOWN);
+ TUR_MSG(msg, MSG_TUR_DOWN);
return PATH_DOWN;
}
- MSG(c, MSG_TUR_UP);
+ TUR_MSG(msg, MSG_TUR_UP);
return PATH_UP;
}

@@ -162,18 +188,25 @@ tur_check (struct checker * c)

void cleanup_func(void *data)
{
+ int holders;
struct tur_checker_context *ct = data;
+ pthread_spin_lock(&ct->hldr_lock);
+ ct->holders--;
+ holders = ct->holders;
ct->thread = 0;
+ pthread_spin_unlock(&ct->hldr_lock);
+ if (!holders)
+ cleanup_context(ct);
}

void *tur_thread(void *ctx)
{
- struct checker *c = ctx;
- struct tur_checker_context *ct = c->context;
+ struct tur_checker_context *ct = ctx;
int state;

condlog(3, "%d:%d: tur checker starting up", TUR_DEVT(ct));

+ ct->message[0] = '';
/* This thread can be canceled, so setup clean up */
tur_thread_cleanup_push(ct)

@@ -182,7 +215,7 @@ void *tur_thread(void *ctx)
ct->state = PATH_PENDING;
pthread_mutex_unlock(&ct->lock);

- state = tur_check(c);
+ state = tur_check(ct->fd, ct->timeout, ct->message);

/* TUR checker done */
pthread_mutex_lock(&ct->lock);
@@ -213,7 +246,7 @@ void tur_set_async_timeout(struct checke
struct timeval now;

gettimeofday(&now, NULL);
- ct->timeout = now.tv_sec + c->timeout;
+ ct->time = now.tv_sec + c->timeout;
}

int tur_check_async_timeout(struct checker *c)
@@ -222,7 +255,7 @@ int tur_check_async_timeout(struct check
struct timeval now;

gettimeofday(&now, NULL);
- return (now.tv_sec > ct->timeout);
+ return (now.tv_sec > ct->time);
}

extern int
@@ -242,7 +275,7 @@ libcheck_check (struct checker * c)
ct->devt = sb.st_rdev;

if (c->sync)
- return tur_check(c);
+ return tur_check(c->fd, c->timeout, c->message);

/*
* Async mode
@@ -276,6 +309,8 @@ libcheck_check (struct checker * c)
/* TUR checker done */
ct->running = 0;
tur_status = ct->state;
+ strncpy(c->message, ct->message, CHECKER_MSG_LEN);
+ c->message[CHECKER_MSG_LEN - 1] = '';
}
pthread_mutex_unlock(&ct->lock);
} else {
@@ -284,24 +319,32 @@ libcheck_check (struct checker * c)
pthread_mutex_unlock(&ct->lock);
condlog(3, "%d:%d: tur thread not responding, "
"using sync mode", TUR_DEVT(ct));
- return tur_check(c);
+ return tur_check(c->fd, c->timeout, c->message);
}
/* Start new TUR checker */
ct->state = PATH_UNCHECKED;
+ ct->fd = c->fd;
+ ct->timeout = c->timeout;
+ pthread_spin_lock(&ct->hldr_lock);
+ ct->holders++;
+ pthread_spin_unlock(&ct->hldr_lock);
tur_set_async_timeout(c);
setup_thread_attr(&attr, 32 * 1024, 1);
- r = pthread_create(&ct->thread, &attr, tur_thread, c);
+ r = pthread_create(&ct->thread, &attr, tur_thread, ct);
if (r) {
pthread_mutex_unlock(&ct->lock);
ct->thread = 0;
+ ct->holders--;
condlog(3, "%d:%d: failed to start tur thread, using"
" sync mode", TUR_DEVT(ct));
- return tur_check(c);
+ return tur_check(c->fd, c->timeout, c->message);
}
pthread_attr_destroy(&attr);
tur_timeout(&tsp);
r = pthread_cond_timedwait(&ct->active, &ct->lock, &tsp);
tur_status = ct->state;
+ strncpy(c->message, ct->message,CHECKER_MSG_LEN);
+ c->message[CHECKER_MSG_LEN -1] = '';
pthread_mutex_unlock(&ct->lock);
if (ct->thread &&
(tur_status == PATH_PENDING || tur_status == PATH_UNCHECKED)) {
Index: multipath-tools-120518/libmultipath/discovery.c
================================================== =================
--- multipath-tools-120518.orig/libmultipath/discovery.c
+++ multipath-tools-120518/libmultipath/discovery.c
@@ -747,6 +747,7 @@ get_state (struct path * pp, int daemon)
}
checker_set_fd(c, pp->fd);
if (checker_init(c, pp->mpp?&pp->mpp->mpcontext:NULL)) {
+ memset(c, 0x0, sizeof(struct checker));
condlog(3, "%s: checker init failed", pp->dev);
return PATH_UNCHECKED;
}

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 11:01 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright ©2007 - 2008, www.linux-archive.org