Re: memory leak and other oddness in pinctrl-mvebu.c
On Sun, Mar 10, 2013 at 1:06 AM, Jason Cooper wrote: > Added LinusW, Gregory and Ezequiel to the email. Guys, can you give > this a Tested-by before I apply (or Ack for LinusW)? If the Marvell people are happy I'm happy. But I would like to have some Tested-by from Sebastian or so? Also I don't quite see the final form of this patch? And shouldn't I rather take the final patch into the pinctrl tree? Then there is a larger issue lurking behind this, and that is that all platforms run their own awkward DT parse code, and that comes from the fact that we could not agree on a common mapping. We must think real hard if we can't atleast share some of the parsing code here. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak and other oddness in pinctrl-mvebu.c
On Sun, Mar 10, 2013 at 1:06 AM, Jason Cooper ja...@lakedaemon.net wrote: Added LinusW, Gregory and Ezequiel to the email. Guys, can you give this a Tested-by before I apply (or Ack for LinusW)? If the Marvell people are happy I'm happy. But I would like to have some Tested-by from Sebastian or so? Also I don't quite see the final form of this patch? And shouldn't I rather take the final patch into the pinctrl tree? Then there is a larger issue lurking behind this, and that is that all platforms run their own awkward DT parse code, and that comes from the fact that we could not agree on a common mapping. We must think real hard if we can't atleast share some of the parsing code here. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak and other oddness in pinctrl-mvebu.c
Added LinusW, Gregory and Ezequiel to the email. Guys, can you give
this a Tested-by before I apply (or Ack for LinusW)?
thx,
Jason.
On Sat, Mar 09, 2013 at 11:39:31PM +, David Woodhouse wrote:
> On Sat, 2013-03-09 at 17:53 -0500, Jason Cooper wrote:
> > > + if (!nr_funcs)
> >
> > shouldn't this be:
> >
> > if (nr_funcs <= 0)
>
> Hm, no. But the loop should terminate if nr_funcs ever reaches zero,
> otherwise funcs->num_groups will be off the end of the original array:
>
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> index c689c04..8bbc607 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> @@ -478,16 +478,21 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
> .dt_free_map = mvebu_pinctrl_dt_free_map,
> };
>
> -static int _add_function(struct mvebu_pinctrl_function *funcs, const char
> *name)
> +static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
> + const char *name)
> {
> - while (funcs->num_groups) {
> + while (nr_funcs && funcs->num_groups) {
> /* function already there */
> if (strcmp(funcs->name, name) == 0) {
> funcs->num_groups++;
> return -EEXIST;
> }
> funcs++;
> + nr_funcs--;
> }
> + if (!nr_funcs)
> + return -EOVERFLOW;
> +
> funcs->name = name;
> funcs->num_groups = 1;
> return 0;
> @@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct
> platform_device *pdev,
> int n, s;
>
> /* we allocate functions for number of pins and hope
> - * there are less unique functions than pins available */
> + * there are fewer unique functions than pins available */
> funcs = devm_kzalloc(>dev, pctl->desc.npins *
>sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
> if (!funcs)
> @@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct
> platform_device *pdev,
> for (n = 0; n < pctl->num_groups; n++) {
> struct mvebu_pinctrl_group *grp = >groups[n];
> for (s = 0; s < grp->num_settings; s++) {
> + int ret;
> +
> /* skip unsupported settings on this variant */
> if (pctl->variant &&
> !(pctl->variant & grp->settings[s].variant))
> continue;
>
> /* check for unique functions and count groups */
> - if (_add_function(funcs, grp->settings[s].name))
> + ret = _add_function(funcs, pctl->desc.npins,
> + grp->settings[s].name);
> + if (ret == -EOVERFLOW)
> + dev_err(>dev,
> + "More functions than pins(%d)\n",
> + pctl->desc.npins);
> + if (ret)
> continue;
>
> num++;
> }
> }
>
> - /* with the number of unique functions and it's groups known,
> -reallocate functions and assign group names */
> - funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
> - GFP_KERNEL);
> - if (!funcs)
> - return -ENOMEM;
> -
> pctl->num_functions = num;
> pctl->functions = funcs;
>
>
> --
> dwmw2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak and other oddness in pinctrl-mvebu.c
On Sat, 2013-03-09 at 17:53 -0500, Jason Cooper wrote:
> > + if (!nr_funcs)
>
> shouldn't this be:
>
> if (nr_funcs <= 0)
Hm, no. But the loop should terminate if nr_funcs ever reaches zero,
otherwise funcs->num_groups will be off the end of the original array:
diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..8bbc607 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,16 +478,21 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
.dt_free_map = mvebu_pinctrl_dt_free_map,
};
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char
*name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
+const char *name)
{
- while (funcs->num_groups) {
+ while (nr_funcs && funcs->num_groups) {
/* function already there */
if (strcmp(funcs->name, name) == 0) {
funcs->num_groups++;
return -EEXIST;
}
funcs++;
+ nr_funcs--;
}
+ if (!nr_funcs)
+ return -EOVERFLOW;
+
funcs->name = name;
funcs->num_groups = 1;
return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
int n, s;
/* we allocate functions for number of pins and hope
-* there are less unique functions than pins available */
+* there are fewer unique functions than pins available */
funcs = devm_kzalloc(>dev, pctl->desc.npins *
sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
if (!funcs)
@@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
for (n = 0; n < pctl->num_groups; n++) {
struct mvebu_pinctrl_group *grp = >groups[n];
for (s = 0; s < grp->num_settings; s++) {
+ int ret;
+
/* skip unsupported settings on this variant */
if (pctl->variant &&
!(pctl->variant & grp->settings[s].variant))
continue;
/* check for unique functions and count groups */
- if (_add_function(funcs, grp->settings[s].name))
+ ret = _add_function(funcs, pctl->desc.npins,
+ grp->settings[s].name);
+ if (ret == -EOVERFLOW)
+ dev_err(>dev,
+ "More functions than pins(%d)\n",
+ pctl->desc.npins);
+ if (ret)
continue;
num++;
}
}
- /* with the number of unique functions and it's groups known,
- reallocate functions and assign group names */
- funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-GFP_KERNEL);
- if (!funcs)
- return -ENOMEM;
-
pctl->num_functions = num;
pctl->functions = funcs;
--
dwmw2
smime.p7s
Description: S/MIME cryptographic signature
Re: memory leak and other oddness in pinctrl-mvebu.c
On Sat, Mar 09, 2013 at 07:02:05PM +, David Woodhouse wrote:
> On Sat, 2013-03-09 at 09:49 +0100, Sebastian Hesselbarth wrote:
> > I don't have a strong opinion on that, but I prefer not to have the list
> > statically in the SoC specific drivers. I think counting the number of
> > unique functions for each SoC specific driver once and verify the above
> > heuristic (fewer unique functions than pins) is still valid. Then drop
> > the krealloc and leave the array the way it is allocated on devm_kzalloc.
>
> Yeah. If you stick a check in the loop and make it warn if it *would*
> have run over the end of the array, that sounds like it ought to be
> fine. Something like this, perhaps? Still untested but otherwise
> Signed-off-by: David Woodhouse
>
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> index c689c04..55d55d5 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> @@ -478,7 +478,8 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
> .dt_free_map = mvebu_pinctrl_dt_free_map,
> };
>
> -static int _add_function(struct mvebu_pinctrl_function *funcs, const char
> *name)
> +static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
> + const char *name)
> {
> while (funcs->num_groups) {
> /* function already there */
> @@ -487,7 +488,11 @@ static int _add_function(struct mvebu_pinctrl_function
> *funcs, const char *name)
> return -EEXIST;
> }
> funcs++;
> + nr_funcs--;
> }
> + if (!nr_funcs)
shouldn't this be:
if (nr_funcs <= 0)
thx,
Jason.
> + return -EOVERFLOW;
> +
> funcs->name = name;
> funcs->num_groups = 1;
> return 0;
> @@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct
> platform_device *pdev,
> int n, s;
>
> /* we allocate functions for number of pins and hope
> - * there are less unique functions than pins available */
> + * there are fewer unique functions than pins available */
> funcs = devm_kzalloc(>dev, pctl->desc.npins *
>sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
> if (!funcs)
> @@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct
> platform_device *pdev,
> for (n = 0; n < pctl->num_groups; n++) {
> struct mvebu_pinctrl_group *grp = >groups[n];
> for (s = 0; s < grp->num_settings; s++) {
> + int ret;
> +
> /* skip unsupported settings on this variant */
> if (pctl->variant &&
> !(pctl->variant & grp->settings[s].variant))
> continue;
>
> /* check for unique functions and count groups */
> - if (_add_function(funcs, grp->settings[s].name))
> + ret = _add_function(funcs, pctl->desc.npins,
> + grp->settings[s].name);
> + if (ret == -EOVERFLOW)
> + dev_err(>dev,
> + "More functions than pins(%d)\n",
> + pctl->desc.npins);
> + if (ret)
> continue;
>
> num++;
> }
> }
>
> - /* with the number of unique functions and it's groups known,
> -reallocate functions and assign group names */
> - funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
> - GFP_KERNEL);
> - if (!funcs)
> - return -ENOMEM;
> -
> pctl->num_functions = num;
> pctl->functions = funcs;
>
>
>
> --
> dwmw2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak and other oddness in pinctrl-mvebu.c
David,
I will not be able to test before mid-week ealiest. I added Andrew Lunn to
the list. He and Thomas can test your patch for Kirkwood and Armada XP/370
respectively. I will test on Dove asap.
Sebastian
On Sat, Mar 9, 2013 at 8:02 PM, David Woodhouse wrote:
> On Sat, 2013-03-09 at 09:49 +0100, Sebastian Hesselbarth wrote:
>> I don't have a strong opinion on that, but I prefer not to have the list
>> statically in the SoC specific drivers. I think counting the number of
>> unique functions for each SoC specific driver once and verify the above
>> heuristic (fewer unique functions than pins) is still valid. Then drop
>> the krealloc and leave the array the way it is allocated on devm_kzalloc.
>
> Yeah. If you stick a check in the loop and make it warn if it *would*
> have run over the end of the array, that sounds like it ought to be
> fine. Something like this, perhaps? Still untested but otherwise
> Signed-off-by: David Woodhouse
>
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> index c689c04..55d55d5 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> @@ -478,7 +478,8 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
> .dt_free_map = mvebu_pinctrl_dt_free_map,
> };
>
> -static int _add_function(struct mvebu_pinctrl_function *funcs, const char
> *name)
> +static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
> +const char *name)
> {
> while (funcs->num_groups) {
> /* function already there */
> @@ -487,7 +488,11 @@ static int _add_function(struct mvebu_pinctrl_function
> *funcs, const char *name)
> return -EEXIST;
> }
> funcs++;
> + nr_funcs--;
> }
> + if (!nr_funcs)
> + return -EOVERFLOW;
> +
> funcs->name = name;
> funcs->num_groups = 1;
> return 0;
> @@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct
> platform_device *pdev,
> int n, s;
>
> /* we allocate functions for number of pins and hope
> -* there are less unique functions than pins available */
> +* there are fewer unique functions than pins available */
> funcs = devm_kzalloc(>dev, pctl->desc.npins *
> sizeof(struct mvebu_pinctrl_function),
> GFP_KERNEL);
> if (!funcs)
> @@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct
> platform_device *pdev,
> for (n = 0; n < pctl->num_groups; n++) {
> struct mvebu_pinctrl_group *grp = >groups[n];
> for (s = 0; s < grp->num_settings; s++) {
> + int ret;
> +
> /* skip unsupported settings on this variant */
> if (pctl->variant &&
> !(pctl->variant & grp->settings[s].variant))
> continue;
>
> /* check for unique functions and count groups */
> - if (_add_function(funcs, grp->settings[s].name))
> + ret = _add_function(funcs, pctl->desc.npins,
> + grp->settings[s].name);
> + if (ret == -EOVERFLOW)
> + dev_err(>dev,
> + "More functions than pins(%d)\n",
> + pctl->desc.npins);
> + if (ret)
> continue;
>
> num++;
> }
> }
>
> - /* with the number of unique functions and it's groups known,
> - reallocate functions and assign group names */
> - funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
> -GFP_KERNEL);
> - if (!funcs)
> - return -ENOMEM;
> -
> pctl->num_functions = num;
> pctl->functions = funcs;
>
>
>
> --
> dwmw2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak and other oddness in pinctrl-mvebu.c
On Sat, 2013-03-09 at 09:49 +0100, Sebastian Hesselbarth wrote:
> I don't have a strong opinion on that, but I prefer not to have the list
> statically in the SoC specific drivers. I think counting the number of
> unique functions for each SoC specific driver once and verify the above
> heuristic (fewer unique functions than pins) is still valid. Then drop
> the krealloc and leave the array the way it is allocated on devm_kzalloc.
Yeah. If you stick a check in the loop and make it warn if it *would*
have run over the end of the array, that sounds like it ought to be
fine. Something like this, perhaps? Still untested but otherwise
Signed-off-by: David Woodhouse
diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..55d55d5 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,7 +478,8 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
.dt_free_map = mvebu_pinctrl_dt_free_map,
};
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char
*name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
+const char *name)
{
while (funcs->num_groups) {
/* function already there */
@@ -487,7 +488,11 @@ static int _add_function(struct mvebu_pinctrl_function
*funcs, const char *name)
return -EEXIST;
}
funcs++;
+ nr_funcs--;
}
+ if (!nr_funcs)
+ return -EOVERFLOW;
+
funcs->name = name;
funcs->num_groups = 1;
return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
int n, s;
/* we allocate functions for number of pins and hope
-* there are less unique functions than pins available */
+* there are fewer unique functions than pins available */
funcs = devm_kzalloc(>dev, pctl->desc.npins *
sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
if (!funcs)
@@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
for (n = 0; n < pctl->num_groups; n++) {
struct mvebu_pinctrl_group *grp = >groups[n];
for (s = 0; s < grp->num_settings; s++) {
+ int ret;
+
/* skip unsupported settings on this variant */
if (pctl->variant &&
!(pctl->variant & grp->settings[s].variant))
continue;
/* check for unique functions and count groups */
- if (_add_function(funcs, grp->settings[s].name))
+ ret = _add_function(funcs, pctl->desc.npins,
+ grp->settings[s].name);
+ if (ret == -EOVERFLOW)
+ dev_err(>dev,
+ "More functions than pins(%d)\n",
+ pctl->desc.npins);
+ if (ret)
continue;
num++;
}
}
- /* with the number of unique functions and it's groups known,
- reallocate functions and assign group names */
- funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-GFP_KERNEL);
- if (!funcs)
- return -ENOMEM;
-
pctl->num_functions = num;
pctl->functions = funcs;
--
dwmw2
smime.p7s
Description: S/MIME cryptographic signature
Re: memory leak and other oddness in pinctrl-mvebu.c
On 03/08/2013 04:58 PM, David Woodhouse wrote: I'm just looking through the kernel for krealloc() abuse, and the 'interesting' code in mvebu_pinctrl_build_functions() came to my attention. First it allocates an array 'funcs' as follows: /* we allocate functions for number of pins and hope * there are less unique functions than pins available */ funcs = devm_kzalloc(>dev, pctl->desc.npins * sizeof(struct mvebu_pinctrl_function), GFP_KERNEL); (note: s/less/fewer/ in the comment). Then it populates the array, seemly trusting to its "hope" and just going off the end of the array if there are more unique functions than pins? And then finally it reallocates the array to the correct size: /* with the number of unique functions and it's groups known, reallocate functions and assign group names */ funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function), GFP_KERNEL); (note: s/it's/its/) So... if krealloc fails, we *leak* the originally-allocated 'funcs'. Except actually we don't because it was allocated with devm_kzalloc() so that's OK. David, the purpose of the above code is to build up a list of unique functions and the pins supporting this function. This is a requirement of the pinctrl API and we had a lengthy discussion with Steven and LinusW back then as the hardware works just the other way round, i.e. a list of pins with a set of functions. The idea was to first assume there will be no more unique functions than pins available and use that array to store the unique function names. After that shrink the array to the actual number of unique functions. BTW, this array isn't used within the driver at all, except to fulfill API requirements for debugfs. If krealloc *succeeds* then I think we should get a double-free because it doesn't free the old 'funcs' via devm_kfree() so when the device is eventually released, won't devres_release_all() free it again? I understand that using devm_kzalloc and krealloc will lead to either double-free or ignored krealloc failure. Is there any other/more correct way of reallocating that pointer? The fix I would prefer is to allocate the array but not realloc it to the correct size. I'm not entirely sure what that krealloc is *for*, anyway. Apart from retrospectively reallocating the array to the size that we've already *scribbled* over? Some kind of request for forgiveness, perhaps? We should probably make the standard kfree() (and hence krealloc()) actually fail when handed pointers which were allocated with devm_kzalloc()? This completely untested patch attempts to fix it by counting the maximum number of functions in advance, then allocating the array at that size. In practice it may overallocate if there are name collisions and _add_function() returns -EEXIST. Is that something we really need to lose sleep over? I agree that counting all potential unique functions is safe here, but will lead to overallocation for sure. Actually, the number of unique functions is known in advance for each SoC but first the information is already there in what the driver uses for controlling the HW (pins with functions) but with multiarch kernels in mind, we chose to built that list on runtime. I don't have a strong opinion on that, but I prefer not to have the list statically in the SoC specific drivers. I think counting the number of unique functions for each SoC specific driver once and verify the above heuristic (fewer unique functions than pins) is still valid. Then drop the krealloc and leave the array the way it is allocated on devm_kzalloc. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak and other oddness in pinctrl-mvebu.c
On 03/08/2013 04:58 PM, David Woodhouse wrote: I'm just looking through the kernel for krealloc() abuse, and the 'interesting' code in mvebu_pinctrl_build_functions() came to my attention. First it allocates an array 'funcs' as follows: /* we allocate functions for number of pins and hope * there are less unique functions than pins available */ funcs = devm_kzalloc(pdev-dev, pctl-desc.npins * sizeof(struct mvebu_pinctrl_function), GFP_KERNEL); (note: s/less/fewer/ in the comment). Then it populates the array, seemly trusting to its hope and just going off the end of the array if there are more unique functions than pins? And then finally it reallocates the array to the correct size: /* with the number of unique functions and it's groups known, reallocate functions and assign group names */ funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function), GFP_KERNEL); (note: s/it's/its/) So... if krealloc fails, we *leak* the originally-allocated 'funcs'. Except actually we don't because it was allocated with devm_kzalloc() so that's OK. David, the purpose of the above code is to build up a list of unique functions and the pins supporting this function. This is a requirement of the pinctrl API and we had a lengthy discussion with Steven and LinusW back then as the hardware works just the other way round, i.e. a list of pins with a set of functions. The idea was to first assume there will be no more unique functions than pins available and use that array to store the unique function names. After that shrink the array to the actual number of unique functions. BTW, this array isn't used within the driver at all, except to fulfill API requirements for debugfs. If krealloc *succeeds* then I think we should get a double-free because it doesn't free the old 'funcs' via devm_kfree() so when the device is eventually released, won't devres_release_all() free it again? I understand that using devm_kzalloc and krealloc will lead to either double-free or ignored krealloc failure. Is there any other/more correct way of reallocating that pointer? The fix I would prefer is to allocate the array but not realloc it to the correct size. I'm not entirely sure what that krealloc is *for*, anyway. Apart from retrospectively reallocating the array to the size that we've already *scribbled* over? Some kind of request for forgiveness, perhaps? We should probably make the standard kfree() (and hence krealloc()) actually fail when handed pointers which were allocated with devm_kzalloc()? This completely untested patch attempts to fix it by counting the maximum number of functions in advance, then allocating the array at that size. In practice it may overallocate if there are name collisions and _add_function() returns -EEXIST. Is that something we really need to lose sleep over? I agree that counting all potential unique functions is safe here, but will lead to overallocation for sure. Actually, the number of unique functions is known in advance for each SoC but first the information is already there in what the driver uses for controlling the HW (pins with functions) but with multiarch kernels in mind, we chose to built that list on runtime. I don't have a strong opinion on that, but I prefer not to have the list statically in the SoC specific drivers. I think counting the number of unique functions for each SoC specific driver once and verify the above heuristic (fewer unique functions than pins) is still valid. Then drop the krealloc and leave the array the way it is allocated on devm_kzalloc. Sebastian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak and other oddness in pinctrl-mvebu.c
On Sat, 2013-03-09 at 09:49 +0100, Sebastian Hesselbarth wrote:
I don't have a strong opinion on that, but I prefer not to have the list
statically in the SoC specific drivers. I think counting the number of
unique functions for each SoC specific driver once and verify the above
heuristic (fewer unique functions than pins) is still valid. Then drop
the krealloc and leave the array the way it is allocated on devm_kzalloc.
Yeah. If you stick a check in the loop and make it warn if it *would*
have run over the end of the array, that sounds like it ought to be
fine. Something like this, perhaps? Still untested but otherwise
Signed-off-by: David Woodhouse david.woodho...@intel.com
diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..55d55d5 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,7 +478,8 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
.dt_free_map = mvebu_pinctrl_dt_free_map,
};
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char
*name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
+const char *name)
{
while (funcs-num_groups) {
/* function already there */
@@ -487,7 +488,11 @@ static int _add_function(struct mvebu_pinctrl_function
*funcs, const char *name)
return -EEXIST;
}
funcs++;
+ nr_funcs--;
}
+ if (!nr_funcs)
+ return -EOVERFLOW;
+
funcs-name = name;
funcs-num_groups = 1;
return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
int n, s;
/* we allocate functions for number of pins and hope
-* there are less unique functions than pins available */
+* there are fewer unique functions than pins available */
funcs = devm_kzalloc(pdev-dev, pctl-desc.npins *
sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
if (!funcs)
@@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
for (n = 0; n pctl-num_groups; n++) {
struct mvebu_pinctrl_group *grp = pctl-groups[n];
for (s = 0; s grp-num_settings; s++) {
+ int ret;
+
/* skip unsupported settings on this variant */
if (pctl-variant
!(pctl-variant grp-settings[s].variant))
continue;
/* check for unique functions and count groups */
- if (_add_function(funcs, grp-settings[s].name))
+ ret = _add_function(funcs, pctl-desc.npins,
+ grp-settings[s].name);
+ if (ret == -EOVERFLOW)
+ dev_err(pdev-dev,
+ More functions than pins(%d)\n,
+ pctl-desc.npins);
+ if (ret)
continue;
num++;
}
}
- /* with the number of unique functions and it's groups known,
- reallocate functions and assign group names */
- funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-GFP_KERNEL);
- if (!funcs)
- return -ENOMEM;
-
pctl-num_functions = num;
pctl-functions = funcs;
--
dwmw2
smime.p7s
Description: S/MIME cryptographic signature
Re: memory leak and other oddness in pinctrl-mvebu.c
David,
I will not be able to test before mid-week ealiest. I added Andrew Lunn to
the list. He and Thomas can test your patch for Kirkwood and Armada XP/370
respectively. I will test on Dove asap.
Sebastian
On Sat, Mar 9, 2013 at 8:02 PM, David Woodhouse dw...@infradead.org wrote:
On Sat, 2013-03-09 at 09:49 +0100, Sebastian Hesselbarth wrote:
I don't have a strong opinion on that, but I prefer not to have the list
statically in the SoC specific drivers. I think counting the number of
unique functions for each SoC specific driver once and verify the above
heuristic (fewer unique functions than pins) is still valid. Then drop
the krealloc and leave the array the way it is allocated on devm_kzalloc.
Yeah. If you stick a check in the loop and make it warn if it *would*
have run over the end of the array, that sounds like it ought to be
fine. Something like this, perhaps? Still untested but otherwise
Signed-off-by: David Woodhouse david.woodho...@intel.com
diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..55d55d5 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,7 +478,8 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
.dt_free_map = mvebu_pinctrl_dt_free_map,
};
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char
*name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
+const char *name)
{
while (funcs-num_groups) {
/* function already there */
@@ -487,7 +488,11 @@ static int _add_function(struct mvebu_pinctrl_function
*funcs, const char *name)
return -EEXIST;
}
funcs++;
+ nr_funcs--;
}
+ if (!nr_funcs)
+ return -EOVERFLOW;
+
funcs-name = name;
funcs-num_groups = 1;
return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
int n, s;
/* we allocate functions for number of pins and hope
-* there are less unique functions than pins available */
+* there are fewer unique functions than pins available */
funcs = devm_kzalloc(pdev-dev, pctl-desc.npins *
sizeof(struct mvebu_pinctrl_function),
GFP_KERNEL);
if (!funcs)
@@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
for (n = 0; n pctl-num_groups; n++) {
struct mvebu_pinctrl_group *grp = pctl-groups[n];
for (s = 0; s grp-num_settings; s++) {
+ int ret;
+
/* skip unsupported settings on this variant */
if (pctl-variant
!(pctl-variant grp-settings[s].variant))
continue;
/* check for unique functions and count groups */
- if (_add_function(funcs, grp-settings[s].name))
+ ret = _add_function(funcs, pctl-desc.npins,
+ grp-settings[s].name);
+ if (ret == -EOVERFLOW)
+ dev_err(pdev-dev,
+ More functions than pins(%d)\n,
+ pctl-desc.npins);
+ if (ret)
continue;
num++;
}
}
- /* with the number of unique functions and it's groups known,
- reallocate functions and assign group names */
- funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-GFP_KERNEL);
- if (!funcs)
- return -ENOMEM;
-
pctl-num_functions = num;
pctl-functions = funcs;
--
dwmw2
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak and other oddness in pinctrl-mvebu.c
On Sat, Mar 09, 2013 at 07:02:05PM +, David Woodhouse wrote:
On Sat, 2013-03-09 at 09:49 +0100, Sebastian Hesselbarth wrote:
I don't have a strong opinion on that, but I prefer not to have the list
statically in the SoC specific drivers. I think counting the number of
unique functions for each SoC specific driver once and verify the above
heuristic (fewer unique functions than pins) is still valid. Then drop
the krealloc and leave the array the way it is allocated on devm_kzalloc.
Yeah. If you stick a check in the loop and make it warn if it *would*
have run over the end of the array, that sounds like it ought to be
fine. Something like this, perhaps? Still untested but otherwise
Signed-off-by: David Woodhouse david.woodho...@intel.com
diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..55d55d5 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,7 +478,8 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
.dt_free_map = mvebu_pinctrl_dt_free_map,
};
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char
*name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
+ const char *name)
{
while (funcs-num_groups) {
/* function already there */
@@ -487,7 +488,11 @@ static int _add_function(struct mvebu_pinctrl_function
*funcs, const char *name)
return -EEXIST;
}
funcs++;
+ nr_funcs--;
}
+ if (!nr_funcs)
shouldn't this be:
if (nr_funcs = 0)
thx,
Jason.
+ return -EOVERFLOW;
+
funcs-name = name;
funcs-num_groups = 1;
return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
int n, s;
/* we allocate functions for number of pins and hope
- * there are less unique functions than pins available */
+ * there are fewer unique functions than pins available */
funcs = devm_kzalloc(pdev-dev, pctl-desc.npins *
sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
if (!funcs)
@@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
for (n = 0; n pctl-num_groups; n++) {
struct mvebu_pinctrl_group *grp = pctl-groups[n];
for (s = 0; s grp-num_settings; s++) {
+ int ret;
+
/* skip unsupported settings on this variant */
if (pctl-variant
!(pctl-variant grp-settings[s].variant))
continue;
/* check for unique functions and count groups */
- if (_add_function(funcs, grp-settings[s].name))
+ ret = _add_function(funcs, pctl-desc.npins,
+ grp-settings[s].name);
+ if (ret == -EOVERFLOW)
+ dev_err(pdev-dev,
+ More functions than pins(%d)\n,
+ pctl-desc.npins);
+ if (ret)
continue;
num++;
}
}
- /* with the number of unique functions and it's groups known,
-reallocate functions and assign group names */
- funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
- GFP_KERNEL);
- if (!funcs)
- return -ENOMEM;
-
pctl-num_functions = num;
pctl-functions = funcs;
--
dwmw2
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: memory leak and other oddness in pinctrl-mvebu.c
On Sat, 2013-03-09 at 17:53 -0500, Jason Cooper wrote:
+ if (!nr_funcs)
shouldn't this be:
if (nr_funcs = 0)
Hm, no. But the loop should terminate if nr_funcs ever reaches zero,
otherwise funcs-num_groups will be off the end of the original array:
diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..8bbc607 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,16 +478,21 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
.dt_free_map = mvebu_pinctrl_dt_free_map,
};
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char
*name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
+const char *name)
{
- while (funcs-num_groups) {
+ while (nr_funcs funcs-num_groups) {
/* function already there */
if (strcmp(funcs-name, name) == 0) {
funcs-num_groups++;
return -EEXIST;
}
funcs++;
+ nr_funcs--;
}
+ if (!nr_funcs)
+ return -EOVERFLOW;
+
funcs-name = name;
funcs-num_groups = 1;
return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
int n, s;
/* we allocate functions for number of pins and hope
-* there are less unique functions than pins available */
+* there are fewer unique functions than pins available */
funcs = devm_kzalloc(pdev-dev, pctl-desc.npins *
sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
if (!funcs)
@@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
for (n = 0; n pctl-num_groups; n++) {
struct mvebu_pinctrl_group *grp = pctl-groups[n];
for (s = 0; s grp-num_settings; s++) {
+ int ret;
+
/* skip unsupported settings on this variant */
if (pctl-variant
!(pctl-variant grp-settings[s].variant))
continue;
/* check for unique functions and count groups */
- if (_add_function(funcs, grp-settings[s].name))
+ ret = _add_function(funcs, pctl-desc.npins,
+ grp-settings[s].name);
+ if (ret == -EOVERFLOW)
+ dev_err(pdev-dev,
+ More functions than pins(%d)\n,
+ pctl-desc.npins);
+ if (ret)
continue;
num++;
}
}
- /* with the number of unique functions and it's groups known,
- reallocate functions and assign group names */
- funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-GFP_KERNEL);
- if (!funcs)
- return -ENOMEM;
-
pctl-num_functions = num;
pctl-functions = funcs;
--
dwmw2
smime.p7s
Description: S/MIME cryptographic signature
Re: memory leak and other oddness in pinctrl-mvebu.c
Added LinusW, Gregory and Ezequiel to the email. Guys, can you give
this a Tested-by before I apply (or Ack for LinusW)?
thx,
Jason.
On Sat, Mar 09, 2013 at 11:39:31PM +, David Woodhouse wrote:
On Sat, 2013-03-09 at 17:53 -0500, Jason Cooper wrote:
+ if (!nr_funcs)
shouldn't this be:
if (nr_funcs = 0)
Hm, no. But the loop should terminate if nr_funcs ever reaches zero,
otherwise funcs-num_groups will be off the end of the original array:
diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..8bbc607 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,16 +478,21 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
.dt_free_map = mvebu_pinctrl_dt_free_map,
};
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char
*name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
+ const char *name)
{
- while (funcs-num_groups) {
+ while (nr_funcs funcs-num_groups) {
/* function already there */
if (strcmp(funcs-name, name) == 0) {
funcs-num_groups++;
return -EEXIST;
}
funcs++;
+ nr_funcs--;
}
+ if (!nr_funcs)
+ return -EOVERFLOW;
+
funcs-name = name;
funcs-num_groups = 1;
return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
int n, s;
/* we allocate functions for number of pins and hope
- * there are less unique functions than pins available */
+ * there are fewer unique functions than pins available */
funcs = devm_kzalloc(pdev-dev, pctl-desc.npins *
sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
if (!funcs)
@@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct
platform_device *pdev,
for (n = 0; n pctl-num_groups; n++) {
struct mvebu_pinctrl_group *grp = pctl-groups[n];
for (s = 0; s grp-num_settings; s++) {
+ int ret;
+
/* skip unsupported settings on this variant */
if (pctl-variant
!(pctl-variant grp-settings[s].variant))
continue;
/* check for unique functions and count groups */
- if (_add_function(funcs, grp-settings[s].name))
+ ret = _add_function(funcs, pctl-desc.npins,
+ grp-settings[s].name);
+ if (ret == -EOVERFLOW)
+ dev_err(pdev-dev,
+ More functions than pins(%d)\n,
+ pctl-desc.npins);
+ if (ret)
continue;
num++;
}
}
- /* with the number of unique functions and it's groups known,
-reallocate functions and assign group names */
- funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
- GFP_KERNEL);
- if (!funcs)
- return -ENOMEM;
-
pctl-num_functions = num;
pctl-functions = funcs;
--
dwmw2
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
