Re: [PATCH v2 1/3] Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus hardening

[Wei Liu <wei.liu@kernel.org>]

On Tue, Jun 30, 2020 at 10:17:36AM +0000, Wei Liu wrote:
[...]
> > > 
> > > If the allocation of the requestor fails during runtime, vmbus_open()
> > > fails too and therefore,
> > > the channel and the requestor will not be created. So, the 2 functions
> > > (next_id, requestor_addr)
> > > will never get called, right? The only case in which we hit this edge
> > > case is if a driver is using this
> > > mechanism with a size of 0 (i.e. rqstor_size is not set to a non-zero
> > > value before calling vmbus_open()),
> > 
> > Right. This is what I was getting at. Setting the size to 0 effectively
> > makes the driver unusable. And per your design, it should be considered
> > a bug.
> > 
> > > but that would be more like a coding bug. So, I think it would be
> > > better to return VMBUS_RQST_ERROR
> > > as a way to assert that there is a bug in the code. I don't know if
> > > I'm missing something here.
> > 
> > Since we know setting size to 0 is a bug, you can actually just do the
> > following in the __vmbus_open function instead of going through all the
> > initialization with the knowledge vmbus_next_request_id & co will fail.
> > 
> >     /* Create and init requestor */
> >     if (!newchannel->rqstor_size)
> >           return an error to caller here
> > 
> >     vmbus_alloc_requestor(...);
> 
> And obviously you should check vmbus_alloc_requestor's return value
> somehow. You get the idea...
> 

Andrea pointed out that I missed one critical aspect of the design --
not all drivers are supposed to use this infrastructure. That's contrary
to my original understanding, in which all drivers are supposed to use
this infrastructure.

With that in mind, it is okay to only initialize the infra only when
->rqstor_size is not zero. Then you just handle the edge case in
vmbus_next_request_id & co.

Wei.

> Wei.
> 
> > 
> > 
> > Wei.
> > 
> > > 
> > > Andres.