In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
failed, we sometime observe panic:
BUG: kernel NULL pointer dereference, address:
...
RIP: 0010:cipso_v4_doi_free+0x3a/0x80
...
Call Trace:
netlbl_cipsov4_add_std+0xf4/0x8c0
netlbl_cipsov4_add+0x13f/0x1b0
genl_family_rcv_msg_doit.isra.15+0x132/0x170
genl_rcv_msg+0x125/0x240
This is because in cipso_v4_doi_free() there is no check
on 'doi_def->map.std' when 'doi_def->type' equal 1, which
is possibe, since netlbl_cipsov4_add_std() haven't initialize
it before alloc 'doi_def->map.std'.
This patch just add the check to prevent panic happen for similar
cases.
Reported-by: Abaci <[email protected]>
Signed-off-by: Michael Wang <[email protected]>
---
net/ipv4/cipso_ipv4.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 099259f..7fbd0b5 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -465,14 +465,16 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
if (!doi_def)
return;
- switch (doi_def->type) {
- case CIPSO_V4_MAP_TRANS:
- kfree(doi_def->map.std->lvl.cipso);
- kfree(doi_def->map.std->lvl.local);
- kfree(doi_def->map.std->cat.cipso);
- kfree(doi_def->map.std->cat.local);
- kfree(doi_def->map.std);
- break;
+ if (doi_def->map.std) {
+ switch (doi_def->type) {
+ case CIPSO_V4_MAP_TRANS:
+ kfree(doi_def->map.std->lvl.cipso);
+ kfree(doi_def->map.std->lvl.local);
+ kfree(doi_def->map.std->cat.cipso);
+ kfree(doi_def->map.std->cat.local);
+ kfree(doi_def->map.std);
+ break;
+ }
}
kfree(doi_def);
}
--
1.8.3.1
On Wed, Aug 25, 2021 at 11:42 PM 王贇 <[email protected]> wrote:
> In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
> failed, we sometime observe panic:
>
> BUG: kernel NULL pointer dereference, address:
> ...
> RIP: 0010:cipso_v4_doi_free+0x3a/0x80
> ...
> Call Trace:
> netlbl_cipsov4_add_std+0xf4/0x8c0
> netlbl_cipsov4_add+0x13f/0x1b0
> genl_family_rcv_msg_doit.isra.15+0x132/0x170
> genl_rcv_msg+0x125/0x240
>
> This is because in cipso_v4_doi_free() there is no check
> on 'doi_def->map.std' when 'doi_def->type' equal 1, which
> is possibe, since netlbl_cipsov4_add_std() haven't initialize
> it before alloc 'doi_def->map.std'.
>
> This patch just add the check to prevent panic happen for similar
> cases.
>
> Reported-by: Abaci <[email protected]>
> Signed-off-by: Michael Wang <[email protected]>
> ---
>
> net/ipv4/cipso_ipv4.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
Thanks for the problem report. It's hard to say for certain due to
the abbreviated backtrace without line number information, but it
looks like the problem you are describing is happening when the
allocation for doi_def->map.std fails near the top of
netlbl_cipsov4_add_std() which causes the function to jump the
add_std_failure target which ends up calling cipso_v4_doi_free().
doi_def = kmalloc(sizeof(*doi_def), GFP_KERNEL);
if (doi_def == NULL)
return -ENOMEM;
doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
if (doi_def->map.std == NULL) {
ret_val = -ENOMEM;
goto add_std_failure;
}
...
add_std_failure:
cipso_v4_doi_free(doi_def);
Since the doi_def allocation is not zero'd out, it is possible that
the doi_def->type value could have a value of CIPSO_V4_MAP_TRANS when
the doi_def->map.std allocation fails, causing the NULL pointer deref
in cipso_v4_doi_free(). As this is the only case where we would see a
problem like this, I suggest a better solution would be to change the
if-block following the doi_def->map.std allocation to something like
this:
doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
if (doi_def->map.std == NULL) {
kfree(doi_def);
return -ENOMEM;
}
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 099259f..7fbd0b5 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -465,14 +465,16 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
> if (!doi_def)
> return;
>
> - switch (doi_def->type) {
> - case CIPSO_V4_MAP_TRANS:
> - kfree(doi_def->map.std->lvl.cipso);
> - kfree(doi_def->map.std->lvl.local);
> - kfree(doi_def->map.std->cat.cipso);
> - kfree(doi_def->map.std->cat.local);
> - kfree(doi_def->map.std);
> - break;
> + if (doi_def->map.std) {
> + switch (doi_def->type) {
> + case CIPSO_V4_MAP_TRANS:
> + kfree(doi_def->map.std->lvl.cipso);
> + kfree(doi_def->map.std->lvl.local);
> + kfree(doi_def->map.std->cat.cipso);
> + kfree(doi_def->map.std->cat.local);
> + kfree(doi_def->map.std);
> + break;
> + }
> }
> kfree(doi_def);
> }
> --
> 1.8.3.1
>
--
paul moore
http://www.paul-moore.com
Just a ping... Should we fix this?
Regards,
Michael Wang
On 2021/8/26 上午11:42, 王贇 wrote:
> In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
> failed, we sometime observe panic:
>
> BUG: kernel NULL pointer dereference, address:
> ...
> RIP: 0010:cipso_v4_doi_free+0x3a/0x80
> ...
> Call Trace:
> netlbl_cipsov4_add_std+0xf4/0x8c0
> netlbl_cipsov4_add+0x13f/0x1b0
> genl_family_rcv_msg_doit.isra.15+0x132/0x170
> genl_rcv_msg+0x125/0x240
>
> This is because in cipso_v4_doi_free() there is no check
> on 'doi_def->map.std' when 'doi_def->type' equal 1, which
> is possibe, since netlbl_cipsov4_add_std() haven't initialize
> it before alloc 'doi_def->map.std'.
>
> This patch just add the check to prevent panic happen for similar
> cases.
>
> Reported-by: Abaci <[email protected]>
> Signed-off-by: Michael Wang <[email protected]>
> ---
>
> net/ipv4/cipso_ipv4.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 099259f..7fbd0b5 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -465,14 +465,16 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
> if (!doi_def)
> return;
>
> - switch (doi_def->type) {
> - case CIPSO_V4_MAP_TRANS:
> - kfree(doi_def->map.std->lvl.cipso);
> - kfree(doi_def->map.std->lvl.local);
> - kfree(doi_def->map.std->cat.cipso);
> - kfree(doi_def->map.std->cat.local);
> - kfree(doi_def->map.std);
> - break;
> + if (doi_def->map.std) {
> + switch (doi_def->type) {
> + case CIPSO_V4_MAP_TRANS:
> + kfree(doi_def->map.std->lvl.cipso);
> + kfree(doi_def->map.std->lvl.local);
> + kfree(doi_def->map.std->cat.cipso);
> + kfree(doi_def->map.std->cat.local);
> + kfree(doi_def->map.std);
> + break;
> + }
> }
> kfree(doi_def);
> }
>
Hi, Paul
I'm sorry for missing this mail since my stupid filter rules...
Will send a new one soon as you suggested :-)
Regards,
Michael Wang
On 2021/8/27 上午8:09, Paul Moore wrote:
[snip]
>>
>> Reported-by: Abaci <[email protected]>
>> Signed-off-by: Michael Wang <[email protected]>
>> ---
>>
>> net/ipv4/cipso_ipv4.c | 18 ++++++++++--------
>> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> Thanks for the problem report. It's hard to say for certain due to
> the abbreviated backtrace without line number information, but it
> looks like the problem you are describing is happening when the
> allocation for doi_def->map.std fails near the top of
> netlbl_cipsov4_add_std() which causes the function to jump the
> add_std_failure target which ends up calling cipso_v4_doi_free().
>
> doi_def = kmalloc(sizeof(*doi_def), GFP_KERNEL);
> if (doi_def == NULL)
> return -ENOMEM;
> doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
> if (doi_def->map.std == NULL) {
> ret_val = -ENOMEM;
> goto add_std_failure;
> }
> ...
> add_std_failure:
> cipso_v4_doi_free(doi_def);
>
> Since the doi_def allocation is not zero'd out, it is possible that
> the doi_def->type value could have a value of CIPSO_V4_MAP_TRANS when
> the doi_def->map.std allocation fails, causing the NULL pointer deref
> in cipso_v4_doi_free(). As this is the only case where we would see a
> problem like this, I suggest a better solution would be to change the
> if-block following the doi_def->map.std allocation to something like
> this:
>
> doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
> if (doi_def->map.std == NULL) {
> kfree(doi_def);
> return -ENOMEM;
> }
>
>> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
>> index 099259f..7fbd0b5 100644
>> --- a/net/ipv4/cipso_ipv4.c
>> +++ b/net/ipv4/cipso_ipv4.c
>> @@ -465,14 +465,16 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
>> if (!doi_def)
>> return;
>>
>> - switch (doi_def->type) {
>> - case CIPSO_V4_MAP_TRANS:
>> - kfree(doi_def->map.std->lvl.cipso);
>> - kfree(doi_def->map.std->lvl.local);
>> - kfree(doi_def->map.std->cat.cipso);
>> - kfree(doi_def->map.std->cat.local);
>> - kfree(doi_def->map.std);
>> - break;
>> + if (doi_def->map.std) {
>> + switch (doi_def->type) {
>> + case CIPSO_V4_MAP_TRANS:
>> + kfree(doi_def->map.std->lvl.cipso);
>> + kfree(doi_def->map.std->lvl.local);
>> + kfree(doi_def->map.std->cat.cipso);
>> + kfree(doi_def->map.std->cat.local);
>> + kfree(doi_def->map.std);
>> + break;
>> + }
>> }
>> kfree(doi_def);
>> }
>> --
>> 1.8.3.1
>>
>
>
In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
failed, we sometime observe panic:
BUG: kernel NULL pointer dereference, address:
...
RIP: 0010:cipso_v4_doi_free+0x3a/0x80
...
Call Trace:
netlbl_cipsov4_add_std+0xf4/0x8c0
netlbl_cipsov4_add+0x13f/0x1b0
genl_family_rcv_msg_doit.isra.15+0x132/0x170
genl_rcv_msg+0x125/0x240
This is because in cipso_v4_doi_free() there is no check
on 'doi_def->map.std' when doi_def->type got value 1, which
is possibe, since netlbl_cipsov4_add_std() haven't initialize
it before alloc 'doi_def->map.std'.
This patch just add the check to prevent panic happen in similar
cases.
Reported-by: Abaci <[email protected]>
Signed-off-by: Michael Wang <[email protected]>
---
net/netlabel/netlabel_cipso_v4.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netlabel/netlabel_cipso_v4.c b/net/netlabel/netlabel_cipso_v4.c
index baf2357..344c228 100644
--- a/net/netlabel/netlabel_cipso_v4.c
+++ b/net/netlabel/netlabel_cipso_v4.c
@@ -144,8 +144,8 @@ static int netlbl_cipsov4_add_std(struct genl_info *info,
return -ENOMEM;
doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
if (doi_def->map.std == NULL) {
- ret_val = -ENOMEM;
- goto add_std_failure;
+ kfree(doi_def);
+ return -ENOMEM;
}
doi_def->type = CIPSO_V4_MAP_TRANS;
--
1.8.3.1
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Mon, 30 Aug 2021 18:28:01 +0800 you wrote:
> In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
> failed, we sometime observe panic:
>
> BUG: kernel NULL pointer dereference, address:
> ...
> RIP: 0010:cipso_v4_doi_free+0x3a/0x80
> ...
> Call Trace:
> netlbl_cipsov4_add_std+0xf4/0x8c0
> netlbl_cipsov4_add+0x13f/0x1b0
> genl_family_rcv_msg_doit.isra.15+0x132/0x170
> genl_rcv_msg+0x125/0x240
>
> [...]
Here is the summary with links:
- [v2] net: fix NULL pointer reference in cipso_v4_doi_free
https://git.kernel.org/netdev/net-next/c/e842cb60e8ac
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Mon, Aug 30, 2021 at 6:28 AM 王贇 <[email protected]> wrote:
>
> In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
> failed, we sometime observe panic:
>
> BUG: kernel NULL pointer dereference, address:
> ...
> RIP: 0010:cipso_v4_doi_free+0x3a/0x80
> ...
> Call Trace:
> netlbl_cipsov4_add_std+0xf4/0x8c0
> netlbl_cipsov4_add+0x13f/0x1b0
> genl_family_rcv_msg_doit.isra.15+0x132/0x170
> genl_rcv_msg+0x125/0x240
>
> This is because in cipso_v4_doi_free() there is no check
> on 'doi_def->map.std' when doi_def->type got value 1, which
> is possibe, since netlbl_cipsov4_add_std() haven't initialize
> it before alloc 'doi_def->map.std'.
>
> This patch just add the check to prevent panic happen in similar
> cases.
>
> Reported-by: Abaci <[email protected]>
> Signed-off-by: Michael Wang <[email protected]>
> ---
> net/netlabel/netlabel_cipso_v4.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
I see this was already merged, but it looks good to me, thanks for
making those changes.
> diff --git a/net/netlabel/netlabel_cipso_v4.c b/net/netlabel/netlabel_cipso_v4.c
> index baf2357..344c228 100644
> --- a/net/netlabel/netlabel_cipso_v4.c
> +++ b/net/netlabel/netlabel_cipso_v4.c
> @@ -144,8 +144,8 @@ static int netlbl_cipsov4_add_std(struct genl_info *info,
> return -ENOMEM;
> doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
> if (doi_def->map.std == NULL) {
> - ret_val = -ENOMEM;
> - goto add_std_failure;
> + kfree(doi_def);
> + return -ENOMEM;
> }
> doi_def->type = CIPSO_V4_MAP_TRANS;
>
> --
> 1.8.3.1
--
paul moore
http://www.paul-moore.com
On Mon, 30 Aug 2021 10:17:05 -0400 Paul Moore wrote:
> On Mon, Aug 30, 2021 at 6:28 AM 王贇 <[email protected]> wrote:
> >
> > In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
> > failed, we sometime observe panic:
> >
> > BUG: kernel NULL pointer dereference, address:
> > ...
> > RIP: 0010:cipso_v4_doi_free+0x3a/0x80
> > ...
> > Call Trace:
> > netlbl_cipsov4_add_std+0xf4/0x8c0
> > netlbl_cipsov4_add+0x13f/0x1b0
> > genl_family_rcv_msg_doit.isra.15+0x132/0x170
> > genl_rcv_msg+0x125/0x240
> >
> > This is because in cipso_v4_doi_free() there is no check
> > on 'doi_def->map.std' when doi_def->type got value 1, which
> > is possibe, since netlbl_cipsov4_add_std() haven't initialize
> > it before alloc 'doi_def->map.std'.
> >
> > This patch just add the check to prevent panic happen in similar
> > cases.
> >
> > Reported-by: Abaci <[email protected]>
> > Signed-off-by: Michael Wang <[email protected]>
> > ---
> > net/netlabel/netlabel_cipso_v4.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
>
> I see this was already merged, but it looks good to me, thanks for
> making those changes.
FWIW it looks like v1 was also merged:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
On Mon, Aug 30, 2021 at 12:45 PM Jakub Kicinski <[email protected]> wrote:
> On Mon, 30 Aug 2021 10:17:05 -0400 Paul Moore wrote:
> > On Mon, Aug 30, 2021 at 6:28 AM 王贇 <[email protected]> wrote:
> > >
> > > In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc
> > > failed, we sometime observe panic:
> > >
> > > BUG: kernel NULL pointer dereference, address:
> > > ...
> > > RIP: 0010:cipso_v4_doi_free+0x3a/0x80
> > > ...
> > > Call Trace:
> > > netlbl_cipsov4_add_std+0xf4/0x8c0
> > > netlbl_cipsov4_add+0x13f/0x1b0
> > > genl_family_rcv_msg_doit.isra.15+0x132/0x170
> > > genl_rcv_msg+0x125/0x240
> > >
> > > This is because in cipso_v4_doi_free() there is no check
> > > on 'doi_def->map.std' when doi_def->type got value 1, which
> > > is possibe, since netlbl_cipsov4_add_std() haven't initialize
> > > it before alloc 'doi_def->map.std'.
> > >
> > > This patch just add the check to prevent panic happen in similar
> > > cases.
> > >
> > > Reported-by: Abaci <[email protected]>
> > > Signed-off-by: Michael Wang <[email protected]>
> > > ---
> > > net/netlabel/netlabel_cipso_v4.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > I see this was already merged, but it looks good to me, thanks for
> > making those changes.
>
> FWIW it looks like v1 was also merged:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
Yeah, that is unfortunate, there was a brief discussion about that
over on one of the -stable patches for the v1 patch (odd that I never
saw a patchbot post for the v1 patch?). Having both merged should be
harmless, but we want to revert the v1 patch as soon as we can.
Michael, can you take care of this?
--
paul moore
http://www.paul-moore.com
On 2021/8/31 上午12:50, Paul Moore wrote:
[SNIP]
>>>> Reported-by: Abaci <[email protected]>
>>>> Signed-off-by: Michael Wang <[email protected]>
>>>> ---
>>>> net/netlabel/netlabel_cipso_v4.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> I see this was already merged, but it looks good to me, thanks for
>>> making those changes.
>>
>> FWIW it looks like v1 was also merged:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
>
> Yeah, that is unfortunate, there was a brief discussion about that
> over on one of the -stable patches for the v1 patch (odd that I never
> saw a patchbot post for the v1 patch?). Having both merged should be
> harmless, but we want to revert the v1 patch as soon as we can.
> Michael, can you take care of this?
As v1 already merged, may be we could just goon with it?
Actually both working to fix the problem, v1 will cover all the
cases, v2 take care one case since that's currently the only one,
but maybe there will be more in future.
Regards,
Michael Wang
>
On Mon, Aug 30, 2021 at 10:42 PM 王贇 <[email protected]> wrote:
> On 2021/8/31 上午12:50, Paul Moore wrote:
> [SNIP]
> >>>> Reported-by: Abaci <[email protected]>
> >>>> Signed-off-by: Michael Wang <[email protected]>
> >>>> ---
> >>>> net/netlabel/netlabel_cipso_v4.c | 4 ++--
> >>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> I see this was already merged, but it looks good to me, thanks for
> >>> making those changes.
> >>
> >> FWIW it looks like v1 was also merged:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
> >
> > Yeah, that is unfortunate, there was a brief discussion about that
> > over on one of the -stable patches for the v1 patch (odd that I never
> > saw a patchbot post for the v1 patch?). Having both merged should be
> > harmless, but we want to revert the v1 patch as soon as we can.
> > Michael, can you take care of this?
>
> As v1 already merged, may be we could just goon with it?
>
> Actually both working to fix the problem, v1 will cover all the
> cases, v2 take care one case since that's currently the only one,
> but maybe there will be more in future.
No. Please revert v1 and stick with the v2 patch. The v1 patch is in
my opinion a rather ugly hack that addresses the symptom of the
problem and not the root cause.
It isn't your fault that both v1 and v2 were merged, but I'm asking
you to help cleanup the mess. If you aren't able to do that please
let us know so that others can fix this properly.
--
paul moore
http://www.paul-moore.com
On 2021/8/31 下午9:48, Paul Moore wrote:
> On Mon, Aug 30, 2021 at 10:42 PM 王贇 <[email protected]> wrote:
>> On 2021/8/31 上午12:50, Paul Moore wrote:
>> [SNIP]
>>>>>> Reported-by: Abaci <[email protected]>
>>>>>> Signed-off-by: Michael Wang <[email protected]>
>>>>>> ---
>>>>>> net/netlabel/netlabel_cipso_v4.c | 4 ++--
>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> I see this was already merged, but it looks good to me, thanks for
>>>>> making those changes.
>>>>
>>>> FWIW it looks like v1 was also merged:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
>>>
>>> Yeah, that is unfortunate, there was a brief discussion about that
>>> over on one of the -stable patches for the v1 patch (odd that I never
>>> saw a patchbot post for the v1 patch?). Having both merged should be
>>> harmless, but we want to revert the v1 patch as soon as we can.
>>> Michael, can you take care of this?
>>
>> As v1 already merged, may be we could just goon with it?
>>
>> Actually both working to fix the problem, v1 will cover all the
>> cases, v2 take care one case since that's currently the only one,
>> but maybe there will be more in future.
>
> No. Please revert v1 and stick with the v2 patch. The v1 patch is in
> my opinion a rather ugly hack that addresses the symptom of the
> problem and not the root cause.
>
> It isn't your fault that both v1 and v2 were merged, but I'm asking
> you to help cleanup the mess. If you aren't able to do that please
> let us know so that others can fix this properly.
No problem I can help on that, just try to make sure it's not a
meaningless work.
So would it be fine to send out a v3 which revert v1 and apply v2?
Regards,
Michael Wang
>
This reverts commit 733c99ee8be9a1410287cdbb943887365e83b2d6.
Since commit e842cb60e8ac ("net: fix NULL pointer reference in
cipso_v4_doi_free") also applied to fix the root cause, we can
just revert the old version now.
Suggested-by: Paul Moore <[email protected]>
Signed-off-by: Michael Wang <[email protected]>
---
net/ipv4/cipso_ipv4.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 7fbd0b5..099259f 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -465,16 +465,14 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
if (!doi_def)
return;
- if (doi_def->map.std) {
- switch (doi_def->type) {
- case CIPSO_V4_MAP_TRANS:
- kfree(doi_def->map.std->lvl.cipso);
- kfree(doi_def->map.std->lvl.local);
- kfree(doi_def->map.std->cat.cipso);
- kfree(doi_def->map.std->cat.local);
- kfree(doi_def->map.std);
- break;
- }
+ switch (doi_def->type) {
+ case CIPSO_V4_MAP_TRANS:
+ kfree(doi_def->map.std->lvl.cipso);
+ kfree(doi_def->map.std->lvl.local);
+ kfree(doi_def->map.std->cat.cipso);
+ kfree(doi_def->map.std->cat.local);
+ kfree(doi_def->map.std);
+ break;
}
kfree(doi_def);
}
--
1.8.3.1
Hi Paul, it confused me since it's the first time I face
such situation, but I just realized what you're asking is
actually this revert, correct?
Regards,
Michael Wang
On 2021/9/1 上午10:18, 王贇 wrote:
> This reverts commit 733c99ee8be9a1410287cdbb943887365e83b2d6.
>
> Since commit e842cb60e8ac ("net: fix NULL pointer reference in
> cipso_v4_doi_free") also applied to fix the root cause, we can
> just revert the old version now.
>
> Suggested-by: Paul Moore <[email protected]>
> Signed-off-by: Michael Wang <[email protected]>
> ---
> net/ipv4/cipso_ipv4.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 7fbd0b5..099259f 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -465,16 +465,14 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
> if (!doi_def)
> return;
>
> - if (doi_def->map.std) {
> - switch (doi_def->type) {
> - case CIPSO_V4_MAP_TRANS:
> - kfree(doi_def->map.std->lvl.cipso);
> - kfree(doi_def->map.std->lvl.local);
> - kfree(doi_def->map.std->cat.cipso);
> - kfree(doi_def->map.std->cat.local);
> - kfree(doi_def->map.std);
> - break;
> - }
> + switch (doi_def->type) {
> + case CIPSO_V4_MAP_TRANS:
> + kfree(doi_def->map.std->lvl.cipso);
> + kfree(doi_def->map.std->lvl.local);
> + kfree(doi_def->map.std->cat.cipso);
> + kfree(doi_def->map.std->cat.local);
> + kfree(doi_def->map.std);
> + break;
> }
> kfree(doi_def);
> }
>
On 2021/9/1 $B2<8a(B5:30, David Miller wrote:
> From: $B2&lV(B <[email protected]>
> Date: Wed, 1 Sep 2021 09:51:28 +0800
>
>>
>>
>> On 2021/8/31 $B2<8a(B9:48, Paul Moore wrote:
>>> On Mon, Aug 30, 2021 at 10:42 PM $B2&lV(B <[email protected]> wrote:
>>>> On 2021/8/31 $B>e8a(B12:50, Paul Moore wrote:
>>>> [SNIP]
>>>>>>>> Reported-by: Abaci <[email protected]>
>>>>>>>> Signed-off-by: Michael Wang <[email protected]>
>>>>>>>> ---
>>>>>>>> net/netlabel/netlabel_cipso_v4.c | 4 ++--
>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> I see this was already merged, but it looks good to me, thanks for
>>>>>>> making those changes.
>>>>>>
>>>>>> FWIW it looks like v1 was also merged:
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
>>>>>
>>>>> Yeah, that is unfortunate, there was a brief discussion about that
>>>>> over on one of the -stable patches for the v1 patch (odd that I never
>>>>> saw a patchbot post for the v1 patch?). Having both merged should be
>>>>> harmless, but we want to revert the v1 patch as soon as we can.
>>>>> Michael, can you take care of this?
>>>>
>>>> As v1 already merged, may be we could just goon with it?
>>>>
>>>> Actually both working to fix the problem, v1 will cover all the
>>>> cases, v2 take care one case since that's currently the only one,
>>>> but maybe there will be more in future.
>>>
>>> No. Please revert v1 and stick with the v2 patch. The v1 patch is in
>>> my opinion a rather ugly hack that addresses the symptom of the
>>> problem and not the root cause.
>>>
>>> It isn't your fault that both v1 and v2 were merged, but I'm asking
>>> you to help cleanup the mess. If you aren't able to do that please
>>> let us know so that others can fix this properly.
>>
>> No problem I can help on that, just try to make sure it's not a
>> meaningless work.
>>
>> So would it be fine to send out a v3 which revert v1 and apply v2?
>
> Please don't do things this way just send the relative change.
Could you please check the patch:
Revert "net: fix NULL pointer reference in cipso_v4_doi_free"
see if that's the right way?
Regards,
Michael Wang
>
> Thanks.
>
From: $B2&lV(B <[email protected]>
Date: Wed, 1 Sep 2021 17:41:00 +0800
>
>
> On 2021/9/1 $B2<8a(B5:30, David Miller wrote:
>> From: $B2&lV(B <[email protected]>
>> Date: Wed, 1 Sep 2021 09:51:28 +0800
>>
>>>
>>>
>>> On 2021/8/31 $B2<8a(B9:48, Paul Moore wrote:
>>>> On Mon, Aug 30, 2021 at 10:42 PM $B2&lV(B <[email protected]> wrote:
>>>>> On 2021/8/31 $B>e8a(B12:50, Paul Moore wrote:
>>>>> [SNIP]
>>>>>>>>> Reported-by: Abaci <[email protected]>
>>>>>>>>> Signed-off-by: Michael Wang <[email protected]>
>>>>>>>>> ---
>>>>>>>>> net/netlabel/netlabel_cipso_v4.c | 4 ++--
>>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> I see this was already merged, but it looks good to me, thanks for
>>>>>>>> making those changes.
>>>>>>>
>>>>>>> FWIW it looks like v1 was also merged:
>>>>>>>
>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
>>>>>>
>>>>>> Yeah, that is unfortunate, there was a brief discussion about that
>>>>>> over on one of the -stable patches for the v1 patch (odd that I never
>>>>>> saw a patchbot post for the v1 patch?). Having both merged should be
>>>>>> harmless, but we want to revert the v1 patch as soon as we can.
>>>>>> Michael, can you take care of this?
>>>>>
>>>>> As v1 already merged, may be we could just goon with it?
>>>>>
>>>>> Actually both working to fix the problem, v1 will cover all the
>>>>> cases, v2 take care one case since that's currently the only one,
>>>>> but maybe there will be more in future.
>>>>
>>>> No. Please revert v1 and stick with the v2 patch. The v1 patch is in
>>>> my opinion a rather ugly hack that addresses the symptom of the
>>>> problem and not the root cause.
>>>>
>>>> It isn't your fault that both v1 and v2 were merged, but I'm asking
>>>> you to help cleanup the mess. If you aren't able to do that please
>>>> let us know so that others can fix this properly.
>>>
>>> No problem I can help on that, just try to make sure it's not a
>>> meaningless work.
>>>
>>> So would it be fine to send out a v3 which revert v1 and apply v2?
>>
>> Please don't do things this way just send the relative change.
>
> Could you please check the patch:
>
> Revert "net: fix NULL pointer reference in cipso_v4_doi_free"
>
> see if that's the right way?
It is not. Please just send a patch against the net GIT tree which relatively changes the code to match v2 of your change.
Thank you.
On Tue, Aug 31, 2021 at 10:21 PM 王贇 <[email protected]> wrote:
>
> Hi Paul, it confused me since it's the first time I face
> such situation, but I just realized what you're asking is
> actually this revert, correct?
I believe DaveM already answered your question in the other thread,
but if you are still unsure about the patch let me know.
--
paul moore
http://www.paul-moore.com
From: $B2&lV(B <[email protected]>
Date: Wed, 1 Sep 2021 09:51:28 +0800
>
>
> On 2021/8/31 $B2<8a(B9:48, Paul Moore wrote:
>> On Mon, Aug 30, 2021 at 10:42 PM $B2&lV(B <[email protected]> wrote:
>>> On 2021/8/31 $B>e8a(B12:50, Paul Moore wrote:
>>> [SNIP]
>>>>>>> Reported-by: Abaci <[email protected]>
>>>>>>> Signed-off-by: Michael Wang <[email protected]>
>>>>>>> ---
>>>>>>> net/netlabel/netlabel_cipso_v4.c | 4 ++--
>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> I see this was already merged, but it looks good to me, thanks for
>>>>>> making those changes.
>>>>>
>>>>> FWIW it looks like v1 was also merged:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
>>>>
>>>> Yeah, that is unfortunate, there was a brief discussion about that
>>>> over on one of the -stable patches for the v1 patch (odd that I never
>>>> saw a patchbot post for the v1 patch?). Having both merged should be
>>>> harmless, but we want to revert the v1 patch as soon as we can.
>>>> Michael, can you take care of this?
>>>
>>> As v1 already merged, may be we could just goon with it?
>>>
>>> Actually both working to fix the problem, v1 will cover all the
>>> cases, v2 take care one case since that's currently the only one,
>>> but maybe there will be more in future.
>>
>> No. Please revert v1 and stick with the v2 patch. The v1 patch is in
>> my opinion a rather ugly hack that addresses the symptom of the
>> problem and not the root cause.
>>
>> It isn't your fault that both v1 and v2 were merged, but I'm asking
>> you to help cleanup the mess. If you aren't able to do that please
>> let us know so that others can fix this properly.
>
> No problem I can help on that, just try to make sure it's not a
> meaningless work.
>
> So would it be fine to send out a v3 which revert v1 and apply v2?
Please don't do things this way just send the relative change.
Thanks.
On 2021/9/2 上午5:05, Paul Moore wrote:
> On Tue, Aug 31, 2021 at 10:21 PM 王贇 <[email protected]> wrote:
>>
>> Hi Paul, it confused me since it's the first time I face
>> such situation, but I just realized what you're asking is
>> actually this revert, correct?
>
> I believe DaveM already answered your question in the other thread,
> but if you are still unsure about the patch let me know.
I do failed to get the point :-(
As I checked the:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
both v1 and v2 are there with the same description and both code modification
are applied.
We want revert v1 but not in a revert patch style, then do you suggest
send a normal patch to do the code revert?
Regards,
Michael Wang
>
On 2021/9/1 $B2<8a(B6:45, David Miller wrote:
[snip]
>>>>>
>>>>> It isn't your fault that both v1 and v2 were merged, but I'm asking
>>>>> you to help cleanup the mess. If you aren't able to do that please
>>>>> let us know so that others can fix this properly.
>>>>
>>>> No problem I can help on that, just try to make sure it's not a
>>>> meaningless work.
>>>>
>>>> So would it be fine to send out a v3 which revert v1 and apply v2?
>>>
>>> Please don't do things this way just send the relative change.
>>
>> Could you please check the patch:
>>
>> Revert "net: fix NULL pointer reference in cipso_v4_doi_free"
>>
>> see if that's the right way?
>
> It is not. Please just send a patch against the net GIT tree which relatively changes the code to match v2 of your change.
Sorry for my horrible reading comprehension... I checked netdev/net.git master branch
and saw v2's change already applied, thus I've no idea how to change it again but pretty
sure I still misunderstanding the suggestion, could please kindly provide more details?
Regards,
Michael Wang
>
> Thank you.
>
On Wed, Sep 1, 2021 at 10:37 PM 王贇 <[email protected]> wrote:
> On 2021/9/2 上午5:05, Paul Moore wrote:
> > On Tue, Aug 31, 2021 at 10:21 PM 王贇 <[email protected]> wrote:
> >>
> >> Hi Paul, it confused me since it's the first time I face
> >> such situation, but I just realized what you're asking is
> >> actually this revert, correct?
> >
> > I believe DaveM already answered your question in the other thread,
> > but if you are still unsure about the patch let me know.
>
> I do failed to get the point :-(
>
> As I checked the:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
>
> both v1 and v2 are there with the same description and both code modification
> are applied.
>
> We want revert v1 but not in a revert patch style, then do you suggest
> send a normal patch to do the code revert?
It sounds like DaveM wants you to create a normal (not a revert) patch
that removes the v1 changes while leaving the v2 changes intact. In
the patch description you can mention that v1 was merged as a mistake
and that v2 is the correct fix (provide commit IDs for each in your
commit description using the usual 12-char hash snippet followed by
the subject in parens-and-quotes).
--
paul moore
http://www.paul-moore.com
On 2021/9/3 上午10:15, Paul Moore wrote:
[snip]
>> both v1 and v2 are there with the same description and both code modification
>> are applied.
>>
>> We want revert v1 but not in a revert patch style, then do you suggest
>> send a normal patch to do the code revert?
>
> It sounds like DaveM wants you to create a normal (not a revert) patch
> that removes the v1 changes while leaving the v2 changes intact. In
> the patch description you can mention that v1 was merged as a mistake
> and that v2 is the correct fix (provide commit IDs for each in your
> commit description using the usual 12-char hash snippet followed by
> the subject in parens-and-quotes).
Thanks for the kindly explain, I've sent:
[PATCH] net: remove the unnecessary check in cipso_v4_doi_free
Which actually revert the v1 and mentioned v2 fixed the root casue,
Would you please take a look see if that is helpful?
Regards,
Michael Wang
>
The commit 733c99ee8be9 ("net: fix NULL pointer reference in
cipso_v4_doi_free") was merged by a mistake, this patch try
to cleanup the mess.
And we already have the commit e842cb60e8ac ("net: fix NULL
pointer reference in cipso_v4_doi_free") which fixed the root
cause of the issue mentioned in it's description.
Suggested-by: Paul Moore <[email protected]>
Signed-off-by: Michael Wang <[email protected]>
---
net/ipv4/cipso_ipv4.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 7fbd0b5..099259f 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -465,16 +465,14 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
if (!doi_def)
return;
- if (doi_def->map.std) {
- switch (doi_def->type) {
- case CIPSO_V4_MAP_TRANS:
- kfree(doi_def->map.std->lvl.cipso);
- kfree(doi_def->map.std->lvl.local);
- kfree(doi_def->map.std->cat.cipso);
- kfree(doi_def->map.std->cat.local);
- kfree(doi_def->map.std);
- break;
- }
+ switch (doi_def->type) {
+ case CIPSO_V4_MAP_TRANS:
+ kfree(doi_def->map.std->lvl.cipso);
+ kfree(doi_def->map.std->lvl.local);
+ kfree(doi_def->map.std->cat.cipso);
+ kfree(doi_def->map.std->cat.local);
+ kfree(doi_def->map.std);
+ break;
}
kfree(doi_def);
}
--
1.8.3.1
On Thu, Sep 2, 2021 at 10:27 PM 王贇 <[email protected]> wrote:
>
> The commit 733c99ee8be9 ("net: fix NULL pointer reference in
> cipso_v4_doi_free") was merged by a mistake, this patch try
> to cleanup the mess.
>
> And we already have the commit e842cb60e8ac ("net: fix NULL
> pointer reference in cipso_v4_doi_free") which fixed the root
> cause of the issue mentioned in it's description.
>
> Suggested-by: Paul Moore <[email protected]>
> Signed-off-by: Michael Wang <[email protected]>
> ---
> net/ipv4/cipso_ipv4.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
Verified that the v2 patch is in net/master so removing the v1 patch
as this does is the right thing to do. Thanks for sending this out.
Acked-by: Paul Moore <[email protected]>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 7fbd0b5..099259f 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -465,16 +465,14 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def)
> if (!doi_def)
> return;
>
> - if (doi_def->map.std) {
> - switch (doi_def->type) {
> - case CIPSO_V4_MAP_TRANS:
> - kfree(doi_def->map.std->lvl.cipso);
> - kfree(doi_def->map.std->lvl.local);
> - kfree(doi_def->map.std->cat.cipso);
> - kfree(doi_def->map.std->cat.local);
> - kfree(doi_def->map.std);
> - break;
> - }
> + switch (doi_def->type) {
> + case CIPSO_V4_MAP_TRANS:
> + kfree(doi_def->map.std->lvl.cipso);
> + kfree(doi_def->map.std->lvl.local);
> + kfree(doi_def->map.std->cat.cipso);
> + kfree(doi_def->map.std->cat.local);
> + kfree(doi_def->map.std);
> + break;
> }
> kfree(doi_def);
> }
> --
> 1.8.3.1
>
--
paul moore
http://www.paul-moore.com
On Thu, Sep 2, 2021 at 10:31 PM 王贇 <[email protected]> wrote:
> On 2021/9/3 上午10:15, Paul Moore wrote:
> [snip]
> >> both v1 and v2 are there with the same description and both code modification
> >> are applied.
> >>
> >> We want revert v1 but not in a revert patch style, then do you suggest
> >> send a normal patch to do the code revert?
> >
> > It sounds like DaveM wants you to create a normal (not a revert) patch
> > that removes the v1 changes while leaving the v2 changes intact. In
> > the patch description you can mention that v1 was merged as a mistake
> > and that v2 is the correct fix (provide commit IDs for each in your
> > commit description using the usual 12-char hash snippet followed by
> > the subject in parens-and-quotes).
>
> Thanks for the kindly explain, I've sent:
> [PATCH] net: remove the unnecessary check in cipso_v4_doi_free
>
> Which actually revert the v1 and mentioned v2 fixed the root casue,
> Would you please take a look see if that is helpful?
That looks correct to me, acked. Thanks.
--
paul moore
http://www.paul-moore.com