Messages in this thread | | | From | Christian Schoenebeck <> | Subject | Re: [PATCH 2/2] net/9p: fix response size check in p9_check_errors() | Date | Tue, 22 Nov 2022 12:20:12 +0100 |
| |
On Tuesday, November 22, 2022 1:21:43 AM CET Dominique Martinet wrote: > Christian Schoenebeck wrote on Tue, Nov 22, 2022 at 12:04:08AM +0100: > > Since 60ece0833b6c (net/9p: allocate appropriate reduced message buffers) > > it is no longer appropriate to check server's response size against > > msize. Check against the previously allocated buffer capacity instead. > > Thanks for the follow up! > > > - Omit this size check entirely for zero-copy messages, as those always > > allocate 4k (P9_ZC_HDR_SZ) linear buffers which are not used for actual > > payload and can be much bigger than 4k. > > [review includes the new flag patch] > > hmm, unless there's anywhere else you think we might use these flags it > looks simpler to just pass a flag to p9_check_errors?
For now that would do as well of course. I just had a feeling that this might be used for other purposes as well in future and some of these functions are already somewhat overloaded with arguments.
No strong opinion, your choice.
> In particular adding a bool in this position is not particularly efficient: > -------(pahole)----- > struct p9_fcall { > u32 size; /* 0 4 */ > u8 id; /* 4 1 */ > > /* XXX 1 byte hole, try to pack */ > > u16 tag; /* 6 2 */ > size_t offset; /* 8 8 */ > size_t capacity; /* 16 8 */ > bool zc; /* 24 1 */ > > /* XXX 7 bytes hole, try to pack */ > > struct kmem_cache * cache; /* 32 8 */ > u8 * sdata; /* 40 8 */ > > /* size: 48, cachelines: 1, members: 8 */ > /* sum members: 40, holes: 2, sum holes: 8 */ > /* last cacheline: 48 bytes */ > }; > ---------------- > Not that adding it between id and tag sounds better to me, so this is > probably just as good as anywhere else :-D
Yeah, that layout optimization would make sense indeed.
> Anyway, I'm just nitpicking -- on principle I agree just whitelisting zc > requests from this check makes most sense, happy with either way if you > think this is better for the future. > > > - Replace p9_debug() by pr_err() to make sure this message is always > > printed in case this error is triggered. > > > > - Add 9p message type to error message to ease investigation. > > Yes to these log changes! > > > > > Signed-off-by: Christian Schoenebeck <linux_oss@crudebyte.com> > > --- > > net/9p/client.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/net/9p/client.c b/net/9p/client.c > > index 30dd82f49b28..63f13dd1ecff 100644 > > --- a/net/9p/client.c > > +++ b/net/9p/client.c > > @@ -514,10 +514,10 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req) > > int ecode; > > > > err = p9_parse_header(&req->rc, NULL, &type, NULL, 0); > > - if (req->rc.size >= c->msize) { > > - p9_debug(P9_DEBUG_ERROR, > > - "requested packet size too big: %d\n", > > - req->rc.size); > > + if (req->rc.size > req->rc.capacity && !req->rc.zc) { > > + pr_err( > > + "requested packet size too big: %d does not fit %ld (type=%d)\n", > > + req->rc.size, req->rc.capacity, req->rc.id); > > Haven't seen this style before -- is that what qemu uses? > We normally keep the message on first line and align e.g.
Lazy me, I haven't run checkpatch.pl this time. I'll fix that.
I also have to fix the format specifier for `capacity` that kernel test bot barked on.
> > + pr_err("requested packet size too big: %d does not fit %ld (type=%d)\n", > > + req->rc.size, req->rc.capacity, req->rc.id); > > (at least what's what other grep -A 1 'pr_err.*,$' seem to do, and > checkpatch is happier with that)
| |