Re: [qemu-s390x] [PATCH v3 1/6] vfio-ccw: make it safe to access channel programs

[Cornelia Huck]

On Tue, 5 Feb 2019 09:41:15 -0500
Eric Farman <address@hidden> wrote:

> On 02/05/2019 07:03 AM, Cornelia Huck wrote:
> > On Mon, 4 Feb 2019 14:25:34 -0500
> > Eric Farman <address@hidden> wrote:
> >   
> >> On 01/30/2019 08:22 AM, Cornelia Huck wrote:  

> >>> @@ -760,6 +764,10 @@ int cp_prefetch(struct channel_program *cp)
> >>>           struct ccwchain *chain;
> >>>           int len, idx, ret;
> >>>    
> >>> + /* this is an error in the caller */
> >>> + if (!cp || !cp->initialized)
> >>> +         return -EINVAL;
> >>> +
> >>>           list_for_each_entry(chain, &cp->ccwchain_list, next) {
> >>>                   len = chain->ch_len;
> >>>                   for (idx = 0; idx < len; idx++) {
> >>> @@ -795,6 +803,10 @@ union orb *cp_get_orb(struct channel_program *cp, 
> >>> u32 intparm, u8 lpm)
> >>>           struct ccwchain *chain;
> >>>           struct ccw1 *cpa;
> >>>    
> >>> + /* this is an error in the caller */
> >>> + if (!cp || !cp->initialized)
> >>> +         return NULL;
> >>> +
> >>>           orb = &cp->orb;
> >>>    
> >>>           orb->cmd.intparm = intparm;
> >>> @@ -831,6 +843,9 @@ void cp_update_scsw(struct channel_program *cp, union 
> >>> scsw *scsw)
> >>>           u32 cpa = scsw->cmd.cpa;
> >>>           u32 ccw_head, ccw_tail;
> >>>    
> >>> + if (!cp->initialized)
> >>> +         return;
> >>> +
> >>>           /*
> >>>            * LATER:
> >>>            * For now, only update the cmd.cpa part. We may need to deal 
> >>> with
> >>> @@ -869,6 +884,9 @@ bool cp_iova_pinned(struct channel_program *cp, u64 
> >>> iova)
> >>>           struct ccwchain *chain;
> >>>           int i;
> >>>    
> >>> + if (!cp->initialized)  
> >>
> >> So, two of the checks added above look for a nonzero cp pointer prior to
> >> checking initialized, while two don't.  I guess cp can't be NULL, since
> >> it's embedded in the private struct directly and that's only free'd when
> >> we do vfio_ccw_sch_remove() ... But I guess some consistency in how we
> >> look would be nice.  
> > 
> > The idea was: In which context is this called? Is there a legitimate
> > reason for the caller to pass in an uninitialized cp, or would that
> > mean the caller had messed up (and we should not trust cp to be !NULL
> > either?)
> > 
> > But you're right, that does look inconsistent. Always checking for
> > cp != NULL probably looks least odd, although it is overkill. Opinions?  
> 
> My opinion?  Since cp is embedded in vfio_ccw_private, rather than a 
> pointer to a separately malloc'd struct, we pass &private->cp to those 
> functions.  So a check for !cp doesn't really buy us anything because 
> what we are actually concerned about is whether or not private is NULL, 
> which only changes on the probe/remove boundaries.

I guess if we pass in crap (or NULL) instead of &private->cp, it's our
own fault and we can disregard fencing that case. The probe/remove path
does not really bother me, for the reasons you said.