Re: [dm-devel] [PATCH v3] multipathd: handle fpin events

[Martin Wilck <martin.wilck@suse.com>]

Hi Ben,

On Thu, 2022-02-10 at 15:02 -0600, Benjamin Marzinski wrote:
> On Thu, Feb 10, 2022 at 05:00:09PM +0000, Martin Wilck wrote:
> > Hi Muneendra, 
> > 
> > coverity found some defects in your patch. Please help me review
> > them,
> > see attachment. It's well possible that they're false positives,
> > but
> > please double-check.
> > 
> > Thanks
> > Martin
> > 
> 

Thank you. Meanwhile I figured this out myself, and marked the defects 
as false positives. I've pushed the patch to my "queue" branch. The
compilation errors have been resolved, too.

> > Date: Thu, 10 Feb 2022 16:50:12 +0000 (UTC)
> > From: scan-admin@coverity.com
> > To: mwilck@suse.com
> > Subject: New Defects reported by Coverity Scan for
> > mwilck/multipath-tools
> > 
> > Hi,
> > 
> > Please find the latest report on new defect(s) introduced to
> > mwilck/multipath-tools found with Coverity Scan.
> > 
> > 3 new defect(s) introduced to mwilck/multipath-tools found with
> > Coverity Scan.
> > 
> > 
> > New defect(s) Reported-by: Coverity Scan
> > Showing 3 of 3 defect(s)
> > 
> > 
> > ** CID 375096:  Memory - corruptions  (OVERRUN)
> > /multipathd/fpin_handlers.c: 161 in fpin_els_add_li_frame()
> > 
> > 
> > ___________________________________________________________________
> > _____________________________________
> > *** CID 375096:  Memory - corruptions  (OVERRUN)
> > /multipathd/fpin_handlers.c: 161 in fpin_els_add_li_frame()
> > 155             pthread_testcancel();
> > 156             els_mrg = calloc(1, sizeof(struct
> > els_marginal_list));
> > 157             if (els_mrg != NULL) {
> > 158                     els_mrg->host_num = fc_event->host_no;
> > 159                     els_mrg->event_code = fc_event->event_code;
> > 160                     els_mrg->length = fc_event->event_datalen;
> > > > >     CID 375096:  Memory - corruptions  (OVERRUN)
> > > > >     Overrunning buffer pointed to by "&fc_event->event_data"
> > > > > of 4 bytes by passing it to a function which accesses it at
> > > > > byte offset 2047 using argument "fc_event->event_datalen"
> > > > > (which evaluates to 2048). [Note: The source code
> > > > > implementation of the function has been overridden by a
> > > > > builtin model.]
> 
> fc_event->event_data is a u32, but that's just because the first 32
> bits
> of the event data is the els_cmd, right? The header makes it clear
> that
> event_data actually does hold event_datalen worth of space
> 
> From /usr/include/scsi/scsi_netlink_fc.h
> -------------------------------------------
>  * Note: if Vendor Unique message, &event_data will be  start of
>  *       vendor unique payload, and the length of the payload is
>  *       per event_datalen
> 
> The only thing that looks possibly suspect here to me is in
> fpin_fabric_notification_receiver() which calls
> fpin_els_add_li_frame()
> 
> In fpin_fabric_notification_receiver() we guarantee that we read
> enough
> for plen to be correct by calling NLMSG_OK(), and we check that plen
> is
> big enough to hold fc_event before we start using that. After that,
> we
> just assume fc_event is well formed. However we could check that
> 
> offsetof(struct fc_nl_event, event_data) + fc_event->event_data_len
> <= plen
> 
> just to make sure that we really read enough space for all the event
> data.
> 
> > 161                     memcpy(els_mrg->payload, &(fc_event-
> > >event_data), fc_event->event_datalen);
> > 162                     list_add_tail(&els_mrg->node,
> > &els_marginal_list_head);
> > 163                     pthread_cond_signal(&fpin_li_cond);
> > 164             } else
> > 165                     ret = -ENOMEM;
> > 166             pthread_cleanup_pop(1);
> > 
> > ** CID 375095:  Program hangs  (LOCK)
> > /multipathd/fpin_handlers.c: 429 in fpin_els_li_consumer()
> > 
> > 
> > ___________________________________________________________________
> > _____________________________________
> > *** CID 375095:  Program hangs  (LOCK)
> > /multipathd/fpin_handlers.c: 429 in fpin_els_li_consumer()
> > 423             rcu_register_thread();
> > 424             pthread_cleanup_push(fpin_clean_marginal_dev_list,
> > NULL);
> > 425             INIT_LIST_HEAD(&marginal_list_head);
> > 426             pthread_cleanup_push(fpin_clean_els_marginal_list,
> > 427                                     (void
> > *)&marginal_list_head);
> > 428             for ( ; ; ) {
> > > > >     CID 375095:  Program hangs  (LOCK)
> > > > >     "pthread_mutex_lock" locks "fpin_li_mutex" while it is
> > > > > locked.
> > 429                     pthread_mutex_lock(&fpin_li_mutex);
> > 430                     pthread_cleanup_push(cleanup_mutex,
> > &fpin_li_mutex);
> > 431                     pthread_testcancel();
> > 432                     while (list_empty(&els_marginal_list_head))
> > 433                             pthread_cond_wait(&fpin_li_cond,
> > &fpin_li_mutex);
> > 434     
> > 
> > ** CID 375094:  Memory - corruptions  (OVERRUN)
> > /multipathd/fpin_handlers.c: 339 in fpin_handle_els_frame()
> 
> I can't see how we could double lock fpin_li_mutex here.  This looks
> to me
> like coverity isn't understanding that the pthread_mutex_pop(1) that
> we
> must do before we loop is unlocking the mutex.

Exactly.

Martin


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