Re: [PATCH 2/4] vfio: ccw: refactor and improve pfn_array_alloc_pin()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 28 Mar 2018 10:36:38 +0800
Dong Jia Shi <bjsdjshi@xxxxxxxxxxxxxxxxxx> wrote:

> * Cornelia Huck <cohuck@xxxxxxxxxx> [2018-03-27 12:01:27 +0200]:
> 
> [...]
> 
> > > > 
> > > > So, basically everything is filled by pfn_array_alloc_pin()?    
> > > Yes.
> > >   
> > > > Should we expect a clean struct pfn_array handed in by the caller,
> > > > then (not just pa_nr == 0)?    
> > > The current idea is:
> > > - It is a clean struct that pfn_array_alloc_pin() expects from its
> > >   caller.
> > > - pfn_array_alloc_pin() and pfn_array_unpin_free() should be used in
> > >   pair. They are the only functions those change the values of the
> > >   elements of a pfn_array struct.
> > > - Caller of pfn_array_alloc_pin() should either hand in a new allocated
> > >   pfn_array (zeroed out), or a freed-after-used one.
> > > - So using pa_nr == 0, is enough to identify all the good cases.
> > >   [We set pa_nr to 0 in pfn_array_unpin_free().]
> > > 
> > > Validating all of the elements only helps when there is case that a
> > > caller breaks the usage rule of these interfaces - the caller itself
> > > assigns values for pfn_pa elements directly... I don't think we allow
> > > this to happen.
> > > 
> > > So I think the current logic is fine.  
> > 
> > Yes, I think it is fine -- I was mainly wondering whether we wanted
> > more sanity checks.
> >   
> Ok.
> Check on (pa->pa_iova_pfn != NULL) could be added. It's easy to do so.
> Check on pa->pa_iova doesn't make sense, since its value will be
> re-assigned anyway.
> Check on pa->pa_pfn doesn't make sense, since we treat it as a pointer
> that points to part of the memory area that was pointed by
> pa->pa_iova_pfn. And we will re-assign it with new pa->pa_iova_pfn
> value.

Yeah, so additional checks are probably not very useful.

> 
> > >   
> > > > 
> > > > Would it make sense to describe the contents of the struct pfn_array
> > > > fields at the struct's definition instead? You could then shorten the
> > > > description here to "we expect pa_nr == 0, any field in this structure
> > > > will be filled in by this function".    
> > > Sounds good!
> > > Do you want a separated patch for this, or I do this change on this
> > > patch? Either will be ok with me.  
> > 
> > Perhaps as an additional patch in front of this one?
> >   
> It's doable. I will do that.
> 

Cool, thx!
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux