Handle access of a target that has been removed

Hello:

I recently got a report of a tgtd core dump from our opencloud group. The stack trace showed that a strcmp against a NULL was causing the failure:

----------------------------------------------------------------
Program terminated with signal 11, Segmentation fault. (gdb) bt
#0 0x00007fa701817576 in __strcmp_sse42 () from /lib64/libc.so.6 #1 0x0000000000408012 in target_find_by_name (

    name=0x6ac16f "iqn.2010-10.org.openstack:volume-e812c705-80bc-4064-a84c-5559cda8b1ca") at iscsi/target.c:216
#2  0x0000000000406042 in login_start (conn=0x6abea8) at iscsi/iscsid.c:478
#3  0x0000000000406e77 in cmnd_exec_login (conn=<optimized out>)
    at iscsi/iscsid.c:654
#4  cmnd_execute (conn=<optimized out>) at iscsi/iscsid.c:914
#5  iscsi_rx_handler (conn=0x6abea8) at iscsi/iscsid.c:2064
#6  0x0000000000409e98 in iscsi_tcp_event_handler (fd=<optimized out>, 
    events=1, data=0x63a480 <target_list>) at iscsi/iscsi_tcp.c:158

#7 0x0000000000418f1e in event_loop () at tgtd.c:272
#8 0x0000000000419405 in main (argc=1, argv=<optimized out>) at tgtd.c:438
----------------------------------------------------------------

It looks like target_find_by_name() uses tgt_targetname(), but doesn't account for the fact that it can return a NULL when the target being looked up does not (now) exist:

----------------------------------------------------------------
char *tgt_targetname(int tid)

{               
        struct target *target;      
                        
        target = target_lookup(tid);
        if (!target)
                return NULL;
        
        return target->name;
}                                   

----------------------------------------------------------------

I propose the following patch to address this:

---

usr/iscsi/target.c +3 -1

index 7afd7fd0ba87..b0a36e7de7b4 100644
@@ -369,9 +369,11 @@ void target_list_build(struct iscsi_connection *conn, char *addr, char *name)
 struct iscsi_target *target_find_by_name(const char *name)
 {
        struct iscsi_target *target;
+       char *tname;
 
        list_for_each_entry(target, &iscsi_targets_list, tlist) {
-               if (!strcmp(tgt_targetname(target->tid), name))
+               tname = tgt_targetname(target->tid);
+               if (tname && !strcmp(tname, name))
                        return target;
        }
 

Signature · 6 lines

-- 
Lee Duncan
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 23 lines · On Tue, 13 Oct 2015 11:10:17 -0700 Lee Duncan <[email protected]> wrote:

Hello:

I recently got a report of a tgtd core dump from our opencloud group. The stack trace showed that a strcmp against a NULL was causing the failure:

----------------------------------------------------------------
Program terminated with signal 11, Segmentation fault. (gdb) bt
#0 0x00007fa701817576 in __strcmp_sse42 () from /lib64/libc.so.6 #1 0x0000000000408012 in target_find_by_name (

    name=0x6ac16f "iqn.2010-10.org.openstack:volume-e812c705-80bc-4064-a84c-5559cda8b1ca") at iscsi/target.c:216
#2  0x0000000000406042 in login_start (conn=0x6abea8) at iscsi/iscsid.c:478
#3  0x0000000000406e77 in cmnd_exec_login (conn=<optimized out>)
    at iscsi/iscsid.c:654
#4  cmnd_execute (conn=<optimized out>) at iscsi/iscsid.c:914
#5  iscsi_rx_handler (conn=0x6abea8) at iscsi/iscsid.c:2064
#6  0x0000000000409e98 in iscsi_tcp_event_handler (fd=<optimized out>, 
    events=1, data=0x63a480 <target_list>) at iscsi/iscsi_tcp.c:158

#7 0x0000000000418f1e in event_loop () at tgtd.c:272
#8 0x0000000000419405 in main (argc=1, argv=<optimized out>) at tgtd.c:438
----------------------------------------------------------------

Quoted 3 lines

It looks like target_find_by_name() uses tgt_targetname(), but doesn't account for the fact that it can return a NULL when the target being looked up does not (now) exist:

Thanks for the patch. But I'm confused why this happens. target with the same tid as iscsi_target must exist?

Quoted 30 lines · FUJITA Tomonori

Quoted 26 lines · On Tue, 13 Oct 2015 11:10:17 -0700 Lee Duncan <[email protected]> wrote:

Hello:

I recently got a report of a tgtd core dump from our opencloud group. The stack trace showed that a strcmp against a NULL was causing the failure:

----------------------------------------------------------------
Program terminated with signal 11, Segmentation fault. (gdb) bt
#0 0x00007fa701817576 in __strcmp_sse42 () from /lib64/libc.so.6 #1 0x0000000000408012 in target_find_by_name (

    name=0x6ac16f "iqn.2010-10.org.openstack:volume-e812c705-80bc-4064-a84c-5559cda8b1ca") at iscsi/target.c:216
#2  0x0000000000406042 in login_start (conn=0x6abea8) at iscsi/iscsid.c:478
#3  0x0000000000406e77 in cmnd_exec_login (conn=<optimized out>)
    at iscsi/iscsid.c:654
#4  cmnd_execute (conn=<optimized out>) at iscsi/iscsid.c:914
#5  iscsi_rx_handler (conn=0x6abea8) at iscsi/iscsid.c:2064
#6  0x0000000000409e98 in iscsi_tcp_event_handler (fd=<optimized out>, 
    events=1, data=0x63a480 <target_list>) at iscsi/iscsi_tcp.c:158

#7 0x0000000000418f1e in event_loop () at tgtd.c:272
#8 0x0000000000419405 in main (argc=1, argv=<optimized out>) at tgtd.c:438
----------------------------------------------------------------

It looks like target_find_by_name() uses tgt_targetname(), but doesn't account for the fact that it can return a NULL when the target being looked up does not (now) exist:

Quoted 3 lines

Thanks for the patch. But I'm confused why this happens. target with the same tid as iscsi_target must exist?

No, I believe this is happening because the targets are getting dynamically removed. But I will verify that. Because if tgt_targetname() can return a NULL (as apparently it did this time), there are probably other places in the code that need to check for that.

Quoted 31 lines · On Mon, 19 Oct 2015 08:56:06 -0700 Lee Duncan <[email protected]> wrote:

Quoted 30 lines · FUJITA Tomonori

Quoted 26 lines · On Tue, 13 Oct 2015 11:10:17 -0700 Lee Duncan <[email protected]> wrote:

Hello:

I recently got a report of a tgtd core dump from our opencloud group. The stack trace showed that a strcmp against a NULL was causing the failure:

----------------------------------------------------------------
Program terminated with signal 11, Segmentation fault. (gdb) bt
#0 0x00007fa701817576 in __strcmp_sse42 () from /lib64/libc.so.6 #1 0x0000000000408012 in target_find_by_name (

    name=0x6ac16f "iqn.2010-10.org.openstack:volume-e812c705-80bc-4064-a84c-5559cda8b1ca") at iscsi/target.c:216
#2  0x0000000000406042 in login_start (conn=0x6abea8) at iscsi/iscsid.c:478
#3  0x0000000000406e77 in cmnd_exec_login (conn=<optimized out>)
    at iscsi/iscsid.c:654
#4  cmnd_execute (conn=<optimized out>) at iscsi/iscsid.c:914
#5  iscsi_rx_handler (conn=0x6abea8) at iscsi/iscsid.c:2064
#6  0x0000000000409e98 in iscsi_tcp_event_handler (fd=<optimized out>, 
    events=1, data=0x63a480 <target_list>) at iscsi/iscsi_tcp.c:158

#7 0x0000000000418f1e in event_loop () at tgtd.c:272
#8 0x0000000000419405 in main (argc=1, argv=<optimized out>) at tgtd.c:438
----------------------------------------------------------------

It looks like target_find_by_name() uses tgt_targetname(), but doesn't account for the fact that it can return a NULL when the target being looked up does not (now) exist:

Quoted 8 lines

Quoted 3 lines

Thanks for the patch. But I'm confused why this happens. target with the same tid as iscsi_target must exist?

No, I believe this is happening because the targets are getting dynamically removed. But I will verify that. Because if tgt_targetname() can return a NULL (as apparently it did this time), there are probably other places in the code that need to check for that.

Ok, I'm still confused but applied the patch. Let's see if it would help.

Thanks,

Quoted 29 lines · FUJITA Tomonori

Quoted 17 lines · On Mon, 19 Oct 2015 08:56:06 -0700 Lee Duncan <[email protected]> wrote:

Quoted 16 lines · FUJITA Tomonori

Quoted 12 lines · On Tue, 13 Oct 2015 11:10:17 -0700 Lee Duncan <[email protected]> wrote:

Hello:

I recently got a report of a tgtd core dump from our opencloud group. The stack trace showed that a strcmp against a NULL was causing the failure:

...

It looks like target_find_by_name() uses tgt_targetname(), but doesn't account for the fact that it can return a NULL when the target being looked up does not (now) exist:

Quoted 8 lines

Quoted 3 lines

Thanks for theI supplied a patch you appliedI supplied a patch you applied patch. But I'm confused why this happens. target with the same tid as iscsi_target must exist?

No, I believe this is happening because the targets are getting dynamically removed. But I will verify that. Because if tgt_targetname() can return a NULL (as apparently it did this time), there are probably other places in the code that need to check for that.

Quoted 4 lines

Ok, I'm still confused but applied the patch. Let's see if it would help.

Thanks,

I am still trying to look more deeply into how this could happen.

This bug was triggered when using cloud storage as the back-end for for their target, and that cloud storage "goes away". (I am still trying to determine what that actually means.) If so, I should be able to simulate by allowing my back-end storage to go away.

As you say, let's see if we see any other instances of tgt_targetname() returning null, in other spots in the code.

I thought that you might like to know that I finally tested this. I

created a test bed where I randomly created and deleted 4 target nodes by changing the configuration files then running "tgt-admin --update ALL -f".

At the same time, I randomly attempted to log out of then back into all 4 targets.

It only takes a minute to hit the case the patch I sent fixes, where target_find_by_name() dies when tgt_targetname() returns an unexpected NULL. It was nice to validate that the patch was actually needed.

You might remember, though, that I was worried that other places where tgt_targetname() where used might need to check for NULL as well. So I tested for that.

Surprisingly, none of the other places that uses tgt_targetname() ever got NULL back. I tested for several hours. So either I just didn't hit the corner case needed to tickle such a bug, or that just doesn't happen. I would love to investigate further, but I may not have the time to get to it soon. In the mean time, it appears we are in good shape.