Re: [PATCH v3 02/11] 9pfs: require msize >= 4096

[Greg Kurz]

On Thu, 16 Jan 2020 22:39:19 +0100
Christian Schoenebeck <address@hidden> wrote:

> On Donnerstag, 16. Januar 2020 19:07:48 CET Greg Kurz wrote:
> > > The point here was that there must be some kind of absolute minimum msize,
> > 
> > Then the absolute minimum size is 7, the size of the header part that is
> > common to all messages:
> > 
> > size[4] Message tag[2]
> > 
> > > even though the protocol specs officially don't mandate one. And if a
> > > client choses a msize < P9_IOHDRSZ, what useful actions shall it be able
> > > to do?
> > I haven't checked but I guess some messages can fit in 24 bytes.
> > 
> > > That's why I mentioned P9_IOHDRSZ just in case somebody might come up
> > > later
> > > asking to lower that minimum msize value for whatever reason.
> > 
> > That's precisely what I want to avoid. The semantic of P9_IOHDRSZ is
> > that it is the header size of a Twrite message and as such it is used
> > to compute the 'iounit' which the guest later uses to compute a
> > suitable 'count' for Tread/Twrite messages only. It shouldn't be
> > involved in msize considerations IMHO.
> > 
> > > But np, IMO this sentence makes the fundamental requirement for this patch
> > > more clear, but I have no problem dropping this sentence.
> > 
> > Then you can mention 7 has the true minimal size.
> 
> Ok, I'll drop P9_IOHDRSZ completely and mention literal 7 as absolute 
> theoretical minimum instead.
> 
> > > > > Furthermore there are some responses which are not allowed by the 9p
> > > > > protocol to be truncated like e.g. Rreadlink which may yield up to
> > > > 
> > > > No message may be truncated in any way actually. The spec just allows
> > > > an exception with the string part of Rerror.
> > > 
> > > Rreaddir, Rread, Twrite, Rerror can be truncated. So I suggest I'll just
> > > change that to s/some/most/ semantically.
> > 
> > I mean truncate with loss of data. Rreaddir can return less than 'count'
> > but it won't truncate an individual entry. It rewinds to the previous
> > one and returns its offset for the next Treaddir. Same goes with Rread
> > and Twrite, we always return a 'count' that can be used by the client
> > to continue reading or writing.
> 
> Individual entries are not truncated, correct, but data loss: Example, you 
> have this directory and say server's fs would deliver entries by readdir() in 
> this order (from top down):
> 
>       .
>       ..
>       a
>       1234567890
>       b
>       c
>       d
> 
> and say 37 >= msize < 45, then client would receive 3 entries ".", "..", "a" 
> and that's it.
> 

Yes msize < 45 isn't enough to send the Rreaddir for "1234567890"
and since it is forbidden to truncate, we bail out... but we
did not send a truncated message still.

> > Rerror is really the only message where we can deliberately drop
> > data (but we actually don't do that).
> > 
> > > > Maybe just mention that and say we choose 4096 to be able to send
> > > > big Rreadlink messages.
> > > 
> > > That's not the point for this patch. The main purpose is to avoid having
> > > to
> > > maintain individual error prone minimum msize checks all over the code
> > > base. If we would allow very small msizes (much smaller than 4096), then
> > > we would need to add numerous error handlers all over the code base, each
> > > one checking for different values (depending on the specific message
> > > type).
> > 
> > I'm not sure what you mean by 'minimum msize checks all over the code
> > base'... Please provide an example.
> 
> 1. Like done in this patch series: Treaddir has to limit client's 'count'
>    parameter according to msize (by subtracting with msize).
> 

Hmm... this patch does a sanity check on 'count', not on 'msize'... I
mean no matter what msize is, clipping count to msize - 11 gives a
chance to stop processing the entries before overflowing the transport
buffer.

> 2. get_iounit() does this:
> 
>               iounit = stbuf.f_bsize;
>               iounit *= (s->msize - P9_IOHDRSZ) / stbuf.f_bsize;
> 
>    without checking anywhere for a potential negative outcome
>    (i.e. when msize < P9_IOHDRSZ)
> 

This function should even have an assert() for that, just to be
sure no one tries to call it before s->msize is even set, which
would clearly be a bug in QEMU. But this can be done in a
follow-up patch.

> 3. Example of directory entry name length with Rreaddir above.
> 

msize being too small to accommodate an Rreaddir with a single
entry is a different problem as we cannot do anything about it
at this point but fail... That's why the minimum msize should
rather be chosen with file names in mind, which are likely to
be longer than any message header. Rreadlink being the one with
the higher potential since it will usually return a string
containing a path name (while Rreaddir entries only contain
a single path element).

> Individual issues that can easily be overlooked but also easily avoided by 
> not 
> allowing small msizes in the first place.
> 

My point is that we're not going to check msize in Tversion in
order to to avoid multiple checks everywhere. We're going to do
it there because it is the only place where it makes sense to
do it.

> Best regards,
> Christian Schoenebeck
> 
>