linux-kernel - Re: [RFC PATCH 07/35] libceph: Change ceph_osdc_call()'s reply to a ceph

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2342310.1742249533@warthog.procyon.org.uk>
Date: Mon, 17 Mar 2025 22:12:13 +0000
From: David Howells <dhowells@...hat.com>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>
Cc: dhowells@...hat.com, Alex Markuze <amarkuze@...hat.com>,
    "slava@...eyko.com" <slava@...eyko.com>,
    "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
    "idryomov@...il.com" <idryomov@...il.com>,
    "jlayton@...nel.org" <jlayton@...nel.org>,
    "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
    "ceph-devel@...r.kernel.org" <ceph-devel@...r.kernel.org>,
    "dongsheng.yang@...ystack.cn" <dongsheng.yang@...ystack.cn>,
    "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 07/35] libceph: Change ceph_osdc_call()'s reply to a ceph_databuf

Viacheslav Dubeyko <Slava.Dubeyko@....com> wrote:

> > +	struct ceph_databuf *reply;
> > +	void *p, *q, *end;
> 
> If I understood correctly the logic, q represents a pointer on current
> position.  So, maybe, it makes sense to rename p into something like
> "begin"? In this case, we will have begin pointer, end pointer and p could
> be used as the name of pointer on current position.

"hdr" might be a better choice.

> > +	iov_iter_advance(&reply->iter, q - p);
> >  
> > -	if (offset_in_page(p) + object_map_bytes > reply_len) {
> > +	if (object_map_bytes > ceph_databuf_len(reply)) {
> 
> Does it mean that we had bug before here? Because it was offset_in_page(p) +
> object_map_bytes before.

No.  The iov_iter_advance() call advances the iterator over the header which
renders the subtraction unnecessary.

> >  	rbd_dev->object_map_size = object_map_size;
> 
> Why do we have object_map_size and object_map_bytes at the same time? It could
> be confusing for my taste. Maybe, we need to rename the object_map_size to
> object_map_num_objects?

Those names preexist.

> > +	reply = ceph_databuf_reply_alloc(1, inbound_size, GFP_KERNEL);
> 
> Interesting... We allocated memory page before. Now we allocate the memory
> of inbound size. Potentially, it could be any size of starting from zero
> bytes and including several memory pages. Could we have an issue here?

Shouldn't do.  ceph_databuf_reply_alloc() will expand databuf's bvec[] as
necessary to accommodate sufficient pages for the requested amount of
bufferage.

David