On 04/17/2013 02:52 PM, Daniel Borkmann wrote:
On 04/17/2013 02:45 PM, Neil Horman wrote:
On Tue, Apr 16, 2013 at 11:07:10PM +0200, Daniel Borkmann wrote:
sctp_ssnmap_init() can only be called from sctp_ssnmap_new()
where malloced is always set to 1. Thus, when we call
sctp_ssnmap_free() the test for map->malloced evaluates always
to true.
Signed-off-by: Daniel Borkmann <dborkman@xxxxxxxxxx>
---
include/net/sctp/structs.h | 1 -
net/sctp/ssnmap.c | 23 ++++++++++++-----------
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index e12aa77..3c1bb8d 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -399,7 +399,6 @@ struct sctp_stream {
struct sctp_ssnmap {
struct sctp_stream in;
struct sctp_stream out;
- int malloced;
};
struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
diff --git a/net/sctp/ssnmap.c b/net/sctp/ssnmap.c
index 825ea94..da86035 100644
--- a/net/sctp/ssnmap.c
+++ b/net/sctp/ssnmap.c
@@ -74,7 +74,6 @@ struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
if (!sctp_ssnmap_init(retval, in, out))
goto fail_map;
- retval->malloced = 1;
SCTP_DBG_OBJCNT_INC(ssnmap);
return retval;
@@ -118,14 +117,16 @@ void sctp_ssnmap_clear(struct sctp_ssnmap *map)
/* Dispose of a ssnmap. */
void sctp_ssnmap_free(struct sctp_ssnmap *map)
{
- if (map && map->malloced) {
- int size;
-
- size = sctp_ssnmap_size(map->in.len, map->out.len);
- if (size <= KMALLOC_MAX_SIZE)
- kfree(map);
- else
- free_pages((unsigned long)map, get_order(size));
- SCTP_DBG_OBJCNT_DEC(ssnmap);
- }
+ int size;
+
+ if (unlikely(!map))
+ return;
+
+ size = sctp_ssnmap_size(map->in.len, map->out.len);
+ if (size <= KMALLOC_MAX_SIZE)
+ kfree(map);
+ else
+ free_pages((unsigned long)map, get_order(size));
+
+ SCTP_DBG_OBJCNT_DEC(ssnmap);
}
--
1.7.11.7
I definately like what you're doing here, as the use of the ->malloced member
always struck me as a half-assed way to try and avoid a double free that someone
couldn't track down during this code's initial development. That said, I'm
wondering if the !map check is going to fail at some point, given that the call
site for sctp_ssnmap_free never sets asoc->ssnmap to NULL after its call. Maybe
worthwhile adding such a NULL assoginment to the call site to ensure that we
don't accidentally trigger a double free?
I'll test that with lksctp-tools suite and come back to you today.
Just did that.
I've poisoned the pointers, so that they would throw a WARN_ON() if they have
already been seen. Also, I've put a WARN_ON() before sctp_ssnmap_new() in
sctp_process_init(), in case asoc->ssnmap was not NULL. I've run the lksctp-tools
suite for v4/v6 and nothing was thrown, also it all passed.
That said, I think that the !map check is there because we init the asoc first
with a NULL ssnmap. I suggest, if Dave wants to and if there are no other
objections, that we could apply to net-next the patches ...
* [1/9] http://patchwork.ozlabs.org/patch/237101/
* [2/9] http://patchwork.ozlabs.org/patch/237102/
* [3/9] http://patchwork.ozlabs.org/patch/237103/
* [5/9] http://patchwork.ozlabs.org/patch/237105/
* [6/9] http://patchwork.ozlabs.org/patch/237109/
* [7/9] http://patchwork.ozlabs.org/patch/237106/
* [8/9] http://patchwork.ozlabs.org/patch/237107/
... as is. I've just tested it, they apply cleanly on top of each other without
the missing. Alternatively, I could resend the set without the two that we cut
out (nr 4 and 9). How you prefer, let me know.
For the remaining two, I think it needs some further analysis, thus I'd say that
we could leave it as is for now and address this at a later point in time.
Thanks,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
