30 Apr
2015
30 Apr
'15
7:28 p.m.
Currently the DL sometimes hangs and sometimes a lot of messages
(still not able to send PDU) are logged. This is caused by an invalid
timer delay computation, setting msecs either to 0 or to some big value.
This is due to an '&' operator at the wrong place, accessing some
parts in fc instead of the first element of the list.
This commit fixes that issue.
Sponsored-by: On-Waves ehf
---
src/gb/gprs_bssgp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/gb/gprs_bssgp.c b/src/gb/gprs_bssgp.c
index 4c93b69..fe4fcca 100644
--- a/src/gb/gprs_bssgp.c
+++ b/src/gb/gprs_bssgp.c
@@ -628,7 +628,7 @@ static int fc_queue_timer_cfg(struct bssgp_flow_control *fc)
if (llist_empty(&fc->queue))
return 0;
- fcqe = llist_entry(&fc->queue.next, struct bssgp_fc_queue_element,
+ fcqe = llist_entry(fc->queue.next, struct bssgp_fc_queue_element,
list);
if (fc->bucket_leak_rate != 0) {
--
1.9.1
30 Apr
30 Apr
8:01 p.m.
On 30 Apr 2015, at 19:28, Jacob Erlbeck
<jerlbeck(a)sysmocom.de> wrote:
Currently the DL sometimes hangs and sometimes a lot of messages
(still not able to send PDU) are logged. This is caused by an invalid
timer delay computation, setting msecs either to 0 or to some big value.
This is due to an '&' operator at the wrong place, accessing some
parts in fc instead of the first element of the list.
ouch. it appears that this is the only occurrence of this issue. How long did
it take for you to find it? Did ASAN help?
8:33 p.m.
On 30.04.2015 20:01, Holger Freyther wrote:
No, since the data seems to be taken from then fc struct, so the memory
area is valid. ASAN was enabled and didn't complain.
Logging helped instead, since the logging messages suggested, that the
timer interval computation might have been broken.
Jacob
--
- Jacob Erlbeck <jerlbeck(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
On 30 Apr 2015, at 19:28, Jacob Erlbeck
<jerlbeck(a)sysmocom.de> wrote:
This is due to an '&' operator at the wrong place, accessing some
parts in fc instead of the first element of the list.
ouch. it appears that this is the only occurrence of this issue. How long did
it take for you to find it? Did ASAN help?
1 May
1 May
12:23 p.m.
On 30.04.2015 20:01, Holger Freyther wrote:
>
>> On 30 Apr 2015, at 19:28, Jacob Erlbeck <jerlbeck(a)sysmocom.de> wrote:
>> This is due to an '&' operator at the wrong place, accessing some
>> parts in fc instead of the first element of the list.
Yikes!
I think the definition of container_of() shouldn't cast ptr:
#define container_of(ptr, type, member) ({ \
- const typeof( ((type *)0)->member ) *__mptr = (typeof( ((type *)0)->member
) *)(ptr); \
+ const typeof( ((type *)0)->member ) *__mptr = (ptr); \
(type *)( (char *)__mptr - offsetof(type, member) );})
Signed-off-by: Michael McTernan <mike.mcternan(a)wavemobile.com>
Then we get one nice warning from gcc:
gprs_bssgp.c: In function 'fc_queue_timer_cfg':
gprs_bssgp.c:631:9: warning: initialization from incompatible pointer type [enabled by
default]
Everything I've tried* still compiles and runs with this change (and generates no
other warnings), but obviously libosmocore could be used somewhere else where container_of
type abuse will now make warnings. If such a case is found where the cast is correct and
required, I'd recommend the casting be placed at the calls to container_of() where it
is more visible and auditable.
Please try the above patch and consider applying it to libosmocore.
Jacob, would you also like/be able to check that the above change is good with Coverity,
incase that can dig out any other bugs?
Kind Regards,
Mike
* I build the following into my system, some of which use libosmocore:
libdbi-0.9.0
libdbi-drivers-0.9.0
libosmo-abis
libosmocore
libosmo-netif
libosmo-sccp
openbsc
openggsn
ortp-0.22.0
osmo-bts
osmo-pcu
4:13 p.m.
On 01 May 2015, at 12:23, Mike McTernan (wavemobile)
<mike.mcternan(a)wavemobile.com> wrote:
I think the definition of container_of() shouldn't cast ptr:
#define container_of(ptr, type, member) ({ \
- const typeof( ((type *)0)->member ) *__mptr = (typeof( ((type *)0)->member
) *)(ptr); \
+ const typeof( ((type *)0)->member ) *__mptr = (ptr); \
(type *)( (char *)__mptr - offsetof(type, member) );})
good point. I thought we have a void* in our hands but we do have the
llist_head here. The container_of macro in linux 2.6.12-rc2 (initial git
commit) didn’t cast either so we should definitely take your commit.
Jacob, would you also like/be able to check that the
above change is good with Coverity, incase that can dig out any other bugs?
Our jenkins only submits HEAD as build to coverity. We don’t have a try
server or similar
5:09 p.m.
Hi Holger,
good point. I thought we have a void* in our hands but
we do have the
llist_head here. The container_of macro in linux 2.6.12-rc2 (initial git
commit) didn’t cast either so we should definitely take your commit.
Great! I subsequently checked too, and couldn't find a version of Linux with the
cast.
The other change to perhaps consider is what happens when the passed ptr is null. In my
projects I tend to use a version of containerof that will return NULL in that case. This
is both convenient for NULL checks and ensures we don't end up with a pointer to the
top of memory (which will _probably_ segfault so would have been detected).
Our jenkins only submits HEAD as build to coverity.
Cool! I just found the Jenkins pages... that's nice to know, thanks.
Kind Regards,
Mike
2 May
2 May
8:03 a.m.
On 01 May 2015, at 12:23, Mike McTernan (wavemobile)
<mike.mcternan(a)wavemobile.com> wrote:
Yikes!
I think the definition of container_of() shouldn't cast ptr:
#define container_of(ptr, type, member) ({ \
- const typeof( ((type *)0)->member ) *__mptr = (typeof( ((type *)0)->member
) *)(ptr); \
+ const typeof( ((type *)0)->member ) *__mptr = (ptr); \
(type *)( (char *)__mptr - offsetof(type, member) );})
In OpenBSC the cast was added in be68f6fc6cde1367a4481d2e774a64e2cd657267
Change the variable "new" to "_new" in order to include it from
C++ code.
The define "container_of" will cast pointer before assigning. Compilers
with stricter options require this. (Andreas Eversberg)
I still think we should revert the cast and see how C++ code complains and then look
at the compiler warning. As you were able to compile osmo-pcu (a C++ app that uses
linked lists) it can’t be that bad.
5 May
5 May
10:25 a.m.
I still think we should revert the cast and see how
C++ code complains
Is anyone suggesting the cast should be kept?
In my view it would be a kind of madness to keep the cast - C is weakly typed as it is, so
removing the little type checking we can get from the compiler is foolhardy and will just
create subtle timewasting bugs. C++ has better typing, generics and true OO, so I
don't see why the cast should be present there, or that container_of should be
particularly useful when better type-safety can be achieved.
and then look at the compiler warning.
The particular warning I highlighted is the one fixed by Jacob's recent patch.
It's something of a proof as to how dangerous that cast is. I'm happy to take a
look at others if they can be found (hence I was wondering about Coverity, but it looks
like we have to submit a change before we can get the results via Jenkins).
As you were able to compile osmo-pcu (a C++ app
that uses linked lists) it can’t be that bad.
Just to recap, I've complied and ran the following without any additional warnings or
problems (though I've not ran all the tests, the libosmocore test all pass without the
cast):
libdbi-0.9.0 libosmo-abis libosmo-netif openggsn osmo-bts
libdbi-drivers-0.9.0 libosmocore libosmo-sccp openbsc ortp-0.22.0 osmo-pcu
sqlite-autoconf-3080802
There are many packages in the osmocom git - are there any other significant projects
which should be checked? Is there anything significant that Jenkins doesn't cover?
Kind Regards,
Mike
11:38 a.m.
On 05 May 2015, at 10:25, Mike McTernan (wavemobile)
<mike.mcternan(a)wavemobile.com> wrote:
no, and I had already pushed a change (outlook mangled your diff so I applied
the change by hand). Thanks for proposing this solution.. i thought we deal with
void* at this point.
http://git.osmocom.org/libosmocore/commit/?id=780bba625d2d09478527ec6038f0f…
I still think we should revert the cast and see
how C++ code complains
Is anyone suggesting the cast should be kept?
4 May
4 May
7:57 a.m.
New subject: [PATCH] bssgp/test: Fix bssgp-fc test
The fix in the commit "bssgp: Fix call to llist_entry in
fc_queue_timer_cfg" prevents the flow control code from generating
certain logging messages ("-BSSGP-FC: fc_timer_cb() but still not
able to send PDU of 10 bytes") during the tests. This breaks the test
suite, since the update of the correseponding stderr output file
misses.
This commit updates the bssgp_fc_tests.err file accordingly.
Sponsored-by: On-Waves ehf
---
tests/gb/bssgp_fc_tests.err | 30 ------------------------------
1 file changed, 30 deletions(-)
diff --git a/tests/gb/bssgp_fc_tests.err b/tests/gb/bssgp_fc_tests.err
index 2f285af..113a359 100644
--- a/tests/gb/bssgp_fc_tests.err
+++ b/tests/gb/bssgp_fc_tests.err
@@ -1,23 +1,3 @@
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
Single PDU (size=1000) is larger than maximum bucket size (100)!
Single PDU (size=1000) is larger than maximum bucket size (100)!
Single PDU (size=1000) is larger than maximum bucket size (100)!
@@ -38,13 +18,3 @@ Single PDU (size=1000) is larger than maximum bucket size (100)!
Single PDU (size=1000) is larger than maximum bucket size (100)!
Single PDU (size=1000) is larger than maximum bucket size (100)!
Single PDU (size=1000) is larger than maximum bucket size (100)!
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
-BSSGP-FC: fc_timer_cb() but still not able to send PDU of 10 bytes
--
1.9.1
3288
days inactive
3293
days old
10 comments
3 participants
participants (3)
-
Holger Freyther -
Jacob Erlbeck -
Mike McTernan (wavemobile)