[PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
dan.carpenter at oracle.com
Fri Mar 16 07:18:58 UTC 2012
On Fri, Mar 16, 2012 at 06:33:35AM +0000, KY Srinivasan wrote:
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter at oracle.com]
> > Sent: Friday, March 16, 2012 1:46 AM
> > To: KY Srinivasan
> > Cc: gregkh at linuxfoundation.org; linux-kernel at vger.kernel.org;
> > devel at linuxdriverproject.org; virtualization at lists.osdl.org; ohering at suse.com;
> > Alan Stern
> > Subject: Re: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP
> > messages in the driver
> > On Thu, Mar 15, 2012 at 05:48:43PM -0700, K. Y. Srinivasan wrote:
> > > /*
> > > * The windows host expects the key/value pair to be encoded
> > > * in utf16.
> > > */
> > > keylen = utf8s_to_utf16s(key_name, strlen(key_name),
> > UTF16_HOST_ENDIAN,
> > > - (wchar_t *) kvp_data->data.key,
> > > + (wchar_t *) kvp_data->key,
> > > HV_KVP_EXCHANGE_MAX_KEY_SIZE / 2);
> > > - kvp_data->data.key_size = 2*(keylen + 1); /* utf16 encoding */
> > > + kvp_data->key_size = 2*(keylen + 1); /* utf16 encoding */
> > > +
> > I feel like a jerk for asking this, but is the output length correct
> > here? It seems like we could go over again. Also utf8s_to_utf16s()
> > can return negative error codes, why do we ignore those?
> We are returning the strings back to the host here. There are checks elsewhere
> in the code to ensure that all strings we return to the host can be accommodated
> in the available space. For the most part these are strings that the host gave us in the
> first place that have already been validated. Furthermore, there are checks on the
> host side to ensure that the returned size parameters are consistent with the protocol
> definitions for the key value pair. For instance let us say somehow we got into a
> situation where the converted utf16 string occupied the entire MAX sized array
> without any room for the terminating character and we set the length parameter
> to 2 more than the MAX value as this code would do. The host would simply discard the
> message as an illegal message. This would be more appropriate than sending a
> truncated key or value.
Uh... Looking at it again, this code is clearly off by one. If
we're not going to hit the limit, then we're not going to truncate,
so that's not a concern. Let's just use the correct limit here.
The problem is that off-by-ones tend to reproduce by copy and paste.
It's best to never introduce any, even harmless ones.
Either that or add a comment. /* Don't care about wrong limitter
because we trust the input. */.
> With regards to the negative values, negative values indicate a failure of some sort
> in the conversion. Since the host is the recipient here, host will correctly deal with the
> transaction by discarding the tuple.
I'm not super familiar with this subsystem. Where can I find code
for rejecting bad transactions? It seems like an easy thing to
handle the error in both places. It makes auditing the code a lot
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 836 bytes
Desc: Digital signature
More information about the devel