On Tue, 2025-03-18 at 22:19 +0000, David Howells wrote:
> Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx> wrote:
>
> > > - ceph_encode_string(&p, end, buf, len);
> > > + BUG_ON(p + sizeof(__le32) + len > end);
> >
> > Frankly speaking, it's hard to follow why sizeof(__le32) should be in the
> > equation. Maybe, it make sense to introduce some constant? The name of
> > constant makes understanding of this calculation more clear.
>
> Look through the patch. It's done all over the place, even on parts I haven't
> touched. However, it's probably because of the way the string is encoded
> (4-byte LE length followed by the characters).
>
> It probably would make sense to use a calculation wrapper for this. I have
> this in fs/afs/yfsclient.c for example:
>
> static size_t xdr_strlen(unsigned int len)
> {
> return sizeof(__be32) + round_up(len, sizeof(__be32));
> }
>
> > > + BUG_ON(sizeof(__le64) + sizeof(__le32) + wsize > req->request->front_alloc_len);
> >
> > The same problem is here. It's hard to follow to this check by involving
> > sizeof(__le64) and sizeof(__le32) in calculation. What these numbers mean here?
>
> Presumably the sizes of the protocol elements in the marshalled data. If you
> want to clean all those up in some way, I can add your patch into my
> series;-).
>
Yeah, I am considering to make the likewise cleanup. :)
Thanks,
Slava.
