| Message ID | 20190925221009.15523-1-navid.emamdoost@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | genetlink: prevent memory leak in netlbl_unlabel_defconf | expand |
On Wed, Sep 25, 2019 at 6:10 PM Navid Emamdoost <navid.emamdoost@gmail.com> wrote: > > In netlbl_unlabel_defconf if netlbl_domhsh_add_default fails the > allocated entry should be released. > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > --- > net/netlabel/netlabel_unlabeled.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) It is worth noting that netlbl_unlabel_defconf() is only called during kernel boot by netlbl_init() and if netlbl_unlabel_defconf() returns an error code the system panics, so this isn't currently a very practical concern. That said, netlbl_unlabel_defconf() *should* clean up here just on principal if nothing else. Acked-by: Paul Moore <paul@paul-moore.com> > diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c > index d2e4ab8d1cb1..c63ec480ee4e 100644 > --- a/net/netlabel/netlabel_unlabeled.c > +++ b/net/netlabel/netlabel_unlabeled.c > @@ -1541,8 +1541,10 @@ int __init netlbl_unlabel_defconf(void) > entry->family = AF_UNSPEC; > entry->def.type = NETLBL_NLTYPE_UNLABELED; > ret_val = netlbl_domhsh_add_default(entry, &audit_info); > - if (ret_val != 0) > + if (ret_val != 0) { > + kfree(entry); > return ret_val; > + } > > netlbl_unlabel_acceptflg_set(1, &audit_info); > > -- > 2.17.1 >
> > In netlbl_unlabel_defconf if netlbl_domhsh_add_default fails the > > allocated entry should be released. … > That said, netlbl_unlabel_defconf() *should* clean up here just on > principal if nothing else. How do you think about to add the tag “Fixes” then? Regards, Markus
On Fri, Sep 27, 2019 at 9:15 AM Markus Elfring <Markus.Elfring@web.de> wrote: > > > > In netlbl_unlabel_defconf if netlbl_domhsh_add_default fails the > > > allocated entry should be released. > … > > That said, netlbl_unlabel_defconf() *should* clean up here just on > > principal if nothing else. > > How do you think about to add the tag “Fixes” then? From what I've seen the "Fixes" tag is typically used by people who are backporting patches, e.g. the -stable folks, to help decide what they need to backport. As I mentioned in my previous email this missing free doesn't actually manifest itself as a practical leak on any of the existing kernels so there isn't a need to backport this patch. For that reason I would probably skip the "Fixes" metadata here, but I don't feel strongly enough about it to object if others want it. FWIW, I play things very conservatively when talking about backporting patches to stable kernels; if it doesn't fix a serious user-visible bug it shouldn't be backported IMHO. This patch is more of a conceptual fix than a practical fix. Not that there is anything wrong with this patch, I just think it isn't as critical as most people would think from reading "memory leak" in the subject line. Yes, there is a memory leak, but the kernel panics soon after so it's a bit moot. Further, even if the panic was somehow skipped (?) the memory leak only happens once during boot; the failed initialization is undoubtedly going to be far more damaging to the system than a few lost bytes of memory.
From: Paul Moore <paul@paul-moore.com> Date: Fri, 27 Sep 2019 10:48:54 -0400 > From what I've seen the "Fixes" tag is typically used by people who > are backporting patches, e.g. the -stable folks, to help decide what > they need to backport. Fixes: tags say what commit introduced the code being fixed, whether it manifests in a real problem or not. It has nothing directly to do with -stable and exists in it's own right whether a change gets backported to -stable or not.
diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c index d2e4ab8d1cb1..c63ec480ee4e 100644 --- a/net/netlabel/netlabel_unlabeled.c +++ b/net/netlabel/netlabel_unlabeled.c @@ -1541,8 +1541,10 @@ int __init netlbl_unlabel_defconf(void) entry->family = AF_UNSPEC; entry->def.type = NETLBL_NLTYPE_UNLABELED; ret_val = netlbl_domhsh_add_default(entry, &audit_info); - if (ret_val != 0) + if (ret_val != 0) { + kfree(entry); return ret_val; + } netlbl_unlabel_acceptflg_set(1, &audit_info);
In netlbl_unlabel_defconf if netlbl_domhsh_add_default fails the allocated entry should be released. Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> --- net/netlabel/netlabel_unlabeled.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)