RE: [PATCH v4] libceph, ceph: reject oversized mon and MDS data segments

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

On Wed, 2026-05-06 at 01:37 +0400, Dhiraj Mishra wrote:
> 
> Hi Slava,
> 
> Yes, both issues can happen on the same message.
> 
> The reason we behave differently is that the two cases are not symmetric.
> 
> For
> 
>         front_len > m->front_alloc_len
> 
> we can still salvage the message, because the front is just a generic byte
> buffer. Replacing m with
> 
>         ceph_msg_new(type, front_len, GFP_NOFS, false)
> 
> gives us a larger front allocation for the same message type.
> 
> For
> 
>         data_len > m->data_length
> 
> there is no analogous generic recovery in this path. ceph_msg_new() here
> is ceph_msg_new2(..., 0, ...), so it creates a message with no data items
> (max_data_items == 0). It only allocates the front buffer; it does not
> provision any data backing. In the tested CEPH_MSG_MON_MAP case, the
> message is intentionally front-only. So if the wire header advertises a
> non-zero data segment, the safe action is to skip the message rather than
> try to invent a data layout.
> 
> So the intended flow is:
> 
> 1. unknown type: skip
> 2. oversized front: reallocate front if possible
> 3. oversized data: validate against the final m and skip if too large
> 
> That is why the data_len check has to stay after the front_len handling.
> If the front_len branch replaces m, we need to validate data_len against
> that final m.
> 
> One small detail in the sketch below: after ceph_msg_new() in the
> front_len branch, we still need to handle the case that it returns NULL,
> so either:
> 
>         if (!m)
>                 return NULL;
> 
> or:
> 
>         if (m && data_len > m->data_length) {
>                 ...
>         }
> 
> is still needed there.

Maybe, I am missing something. But I don't quite follow to you.

We have checked that m is NULL here and exit the method:

if (!m) {
    pr_info("alloc_msg unknown type %d\n", type);
    *skip = 1;
    return m;
}

Now we have valid m pointer. Potentially, ceph_msg_new() could return NULL. And
we simply can check it here:

if (front_len > m->front_alloc_len) {
                pr_warn("mon_alloc_msg front %d > prealloc %d (%u#%llu)\n",
                        front_len, m->front_alloc_len,
                        (unsigned int)con->peer_name.type,
                        le64_to_cpu(con->peer_name.num));
                ceph_msg_put(m);
                m = ceph_msg_new(type, front_len, GFP_NOFS, false);
                if (!m)
                    return m;
}

And we should have valid m here again:

if (data_len > m->data_length) {
		pr_warn_ratelimited("mon message data %u > prealloc %zu
(%u#%llu), skipping\n",
				    data_len, m->data_length,
				    (unsigned int)con->peer_name.type,
				    le64_to_cpu(con->peer_name.num));
		ceph_msg_put(m);
		m = NULL;
		*skip = 1;
}

I am simply trying to make the code more clean and without introducing new
errors.

Do we need to check both front_len and data_len? I assume that these fields work
in different situations. Am I right? Could we make these checks more local in
the switch?

Thanks,
Slava.