lkml.org 
[lkml]   [2012]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/3] Drivers: hv: Support the newly introduced KVP messages in the driver
    On Fri, Mar 16, 2012 at 06:33:35AM +0000, KY Srinivasan wrote:
    >
    >
    > > -----Original Message-----
    > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
    > > Sent: Friday, March 16, 2012 1:46 AM
    > > To: KY Srinivasan
    > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
    > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@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
    simpler.

    regards,
    dan carpenter

    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2012-03-16 08:19    [W:0.037 / U:0.072 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site