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

[Christian Schoenebeck]

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.

> 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).

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)

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

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

Best regards,
Christian Schoenebeck