netgraph/ng_checksum: Fix double free error

[donner]

Summary

m_pullup(9) frees the mbuf(9) chain in the case of an allocation error.
The mbuf chain must not be freed again at the end in this case.

PR: 255874
Submitted by: <[e-mail]>
MFC after: 1 week

Test Plan

Tested by the submitter.

[markj]

checksum_ipv4() and checksum_ipv6() may fail without freeing the mbuf. In that case, with the patch the mbuf is leaked.

One approach that's used in some places is to pass a struct mbuf ** to the subroutine, and have the callee set *m = NULL when it frees the mbuf. Alternately, checksum_ip*() can be modified to always free the mbuf upon failure.

sys/netgraph/ng_checksum.c

686

drop is not really an accurate name for the label now.

[donner]

In D30273#680113, @markj wrote:

checksum_ipv4() and checksum_ipv6() may fail without freeing the mbuf. In that case, with the patch the mbuf is leaked.

Can you point to a code line, where this can happen?
I didn't see one.

One approach that's used in some places is to pass a struct mbuf ** to the subroutine,
and have the callee set *m = NULL when it frees the mbuf. Alternately, checksum_ip*()
can be modified to always free the mbuf upon failure.

As part of a brushup of this code, yes.
As part of a bug fix, no.

sys/netgraph/ng_checksum.c

686

Not as part of a bugfix.

[markj]

In D30273#680142, @donner wrote:

In D30273#680113, @markj wrote:

checksum_ipv4() and checksum_ipv6() may fail without freeing the mbuf. In that case, with the patch the mbuf is leaked.

Can you point to a code line, where this can happen?
I didn't see one.

In checksum_ipv4() we call PULLUP_CHECK() on line 316. PULLUP_CHECK() looks like this:

293 #define PULLUP_CHECK(mbuf, length) do {                                 \                                                                                                 
294         pullup_len += length;                                           \                                                                                                 
295         if (((mbuf)->m_pkthdr.len < pullup_len) ||                      \                                                                                                 
296             (pullup_len > MHLEN)) {                                     \                                                                                                 
297                 return (EINVAL);                                        \                                                                                                 
298         }                                                               \                                                                                                 
299         if ((mbuf)->m_len < pullup_len &&                               \                                                                                                 
300             (((mbuf) = m_pullup((mbuf), pullup_len)) == NULL)) {        \                                                                                                 
301                 return (ENOBUFS);                                       \                                                                                                 
302         }                                                               \                                                                                                 
303 } while (0)

On line 297, we return an error without having freed the mbuf. On line 301 we return an error having freed the mbuf, and the caller of checksum_ipv4() will free the mbuf again. So in the first case, we are now not freeing the mbuf at all.

[markj]

In D30273#680142, @donner wrote:

In D30273#680113, @markj wrote:

One approach that's used in some places is to pass a struct mbuf ** to the subroutine,
and have the callee set *m = NULL when it frees the mbuf. Alternately, checksum_ip*()
can be modified to always free the mbuf upon failure.

As part of a brushup of this code, yes.
As part of a bug fix, no.

To be clear, what I am proposing there is part of the bug fix, not a cleanup.

[donner]

In D30273#680148, @markj wrote:

In D30273#680142, @donner wrote:

In D30273#680113, @markj wrote:

checksum_ipv4() and checksum_ipv6() may fail without freeing the mbuf. In that case, with the patch the mbuf is leaked.

Can you point to a code line, where this can happen?
I didn't see one.

In checksum_ipv4() we call PULLUP_CHECK() on line 316. PULLUP_CHECK() looks like this:

293 #define PULLUP_CHECK(mbuf, length) do {                                 \                                                                                                 
294         pullup_len += length;                                           \                                                                                                 
295         if (((mbuf)->m_pkthdr.len < pullup_len) ||                      \                                                                                                 
296             (pullup_len > MHLEN)) {                                     \                                                                                                 
297                 return (EINVAL);                                        \                                                                                                 
298         }                                                               \                                                                                                 
299         if ((mbuf)->m_len < pullup_len &&                               \                                                                                                 
300             (((mbuf) = m_pullup((mbuf), pullup_len)) == NULL)) {        \                                                                                                 
301                 return (ENOBUFS);                                       \                                                                                                 
302         }                                                               \                                                                                                 
303 } while (0)

On line 297, we return an error without having freed the mbuf. On line 301 we return an error having freed the mbuf, and the caller of checksum_ipv4() will free the mbuf again. So in the first case, we are now not freeing the mbuf at all.

This is well respected on the caller side:

	error = checksum_ipv*(priv, m, pullup_len);
	if (error == 0)
		goto bypass;
	else if (error == ENOBUFS)
		goto drop;

I don't say, the bug fix could be improved, ...

[markj]

In D30273#680188, @donner wrote:

In D30273#680148, @markj wrote:

In D30273#680142, @donner wrote:

In D30273#680113, @markj wrote:

checksum_ipv4() and checksum_ipv6() may fail without freeing the mbuf. In that case, with the patch the mbuf is leaked.

Can you point to a code line, where this can happen?
I didn't see one.

In checksum_ipv4() we call PULLUP_CHECK() on line 316. PULLUP_CHECK() looks like this:

293 #define PULLUP_CHECK(mbuf, length) do {                                 \                                                                                                 
294         pullup_len += length;                                           \                                                                                                 
295         if (((mbuf)->m_pkthdr.len < pullup_len) ||                      \                                                                                                 
296             (pullup_len > MHLEN)) {                                     \                                                                                                 
297                 return (EINVAL);                                        \                                                                                                 
298         }                                                               \                                                                                                 
299         if ((mbuf)->m_len < pullup_len &&                               \                                                                                                 
300             (((mbuf) = m_pullup((mbuf), pullup_len)) == NULL)) {        \                                                                                                 
301                 return (ENOBUFS);                                       \                                                                                                 
302         }                                                               \                                                                                                 
303 } while (0)

On line 297, we return an error without having freed the mbuf. On line 301 we return an error having freed the mbuf, and the caller of checksum_ipv4() will free the mbuf again. So in the first case, we are now not freeing the mbuf at all.

This is well respected on the caller side:

	error = checksum_ipv*(priv, m, pullup_len);
	if (error == 0)
		goto bypass;
	else if (error == ENOBUFS)
		goto drop;

I don't say, the bug fix could be improved, ...

Sorry, I missed this detail when reading the code. I think the diff is fine.