locking errors in IPF in netbsd-6

[Chuck Silvers]

recently I had occasion to run "ipfstat -f" (which I had never run before)
on a netbsd-6 system, and it immediately rebooted. no dump device configured, alas.

I tried it again later on a test system and ipfstat hung instead:

db{0}> t/a fffffe8429568680
trace: pid 328 lid 1 at 0xfffffe811e2e4d00
sleepq_block() at netbsd:sleepq_block+0xad
turnstile_block() at netbsd:turnstile_block+0x2bb
rw_vector_enter() at netbsd:rw_vector_enter+0x1eb
ipf_findtoken() at netbsd:ipf_findtoken+0x44
fr_nat_ioctl() at netbsd:fr_nat_ioctl+0x4a3
cdev_ioctl() at netbsd:cdev_ioctl+0x77
VOP_IOCTL() at netbsd:VOP_IOCTL+0x3b
vn_ioctl() at netbsd:vn_ioctl+0x76
sys_ioctl() at netbsd:sys_ioctl+0x13c
syscall() at netbsd:syscall+0xc4


there are some obvious locking problems to do with "ipf_tokens"
in the version of IPF in the 6.x branch, this lock can be unlocked
when it's not held. does the attached patch look correct?

(the more recent version of IPF in -current doesn't have these problems)

-Chuck

[David Laight]

-/* */
-/* NOTE: It is by design that this function returns holding a read lock on */
-/* ipf_tokens. Callers must make sure they release it! */
/* ----------------------------------------------------------------------- */

Does that comment really mean that the non-NULL return value might
become invalid as soon as the lock is released?

Which would mean that you've removed the wrong unlock!

David

[Chuck Silvers]

Post by David Laight

-/* */
-/* NOTE: It is by design that this function returns holding a read lock on */
-/* ipf_tokens. Callers must make sure they release it! */
/* ----------------------------------------------------------------------- */

Does that comment really mean that the non-NULL return value might
become invalid as soon as the lock is released?

looking at what the callers do, it appears that this comment is no longer correct.
with my patch, I think the code now matches the locking logic in the newer version
of IPF in -current, and that comment was removed from the corresponding function
there, thus my patch removes it as well.

-Chuck

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Darren Reed]

Post by Chuck Silvers
recently I had occasion to run "ipfstat -f" (which I had never run before)
on a netbsd-6 system, and it immediately rebooted. no dump device configured, alas.
db{0}> t/a fffffe8429568680
trace: pid 328 lid 1 at 0xfffffe811e2e4d00
sleepq_block() at netbsd:sleepq_block+0xad
turnstile_block() at netbsd:turnstile_block+0x2bb
rw_vector_enter() at netbsd:rw_vector_enter+0x1eb
ipf_findtoken() at netbsd:ipf_findtoken+0x44
fr_nat_ioctl() at netbsd:fr_nat_ioctl+0x4a3
cdev_ioctl() at netbsd:cdev_ioctl+0x77
VOP_IOCTL() at netbsd:VOP_IOCTL+0x3b
vn_ioctl() at netbsd:vn_ioctl+0x76
sys_ioctl() at netbsd:sys_ioctl+0x13c
syscall() at netbsd:syscall+0xc4
there are some obvious locking problems to do with "ipf_tokens"
in the version of IPF in the 6.x branch, this lock can be unlocked
when it's not held. does the attached patch look correct?
(the more recent version of IPF in -current doesn't have these problems)
-Chuck

Chuck,

Thanks for picking that up.
You're right in that the comment is wrong.

Cheers,
Darren


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Chuck Silvers]

Post by Darren Reed

Post by Chuck Silvers
recently I had occasion to run "ipfstat -f" (which I had never run before)
on a netbsd-6 system, and it immediately rebooted. no dump device configured, alas.
db{0}> t/a fffffe8429568680
trace: pid 328 lid 1 at 0xfffffe811e2e4d00
sleepq_block() at netbsd:sleepq_block+0xad
turnstile_block() at netbsd:turnstile_block+0x2bb
rw_vector_enter() at netbsd:rw_vector_enter+0x1eb
ipf_findtoken() at netbsd:ipf_findtoken+0x44
fr_nat_ioctl() at netbsd:fr_nat_ioctl+0x4a3
cdev_ioctl() at netbsd:cdev_ioctl+0x77
VOP_IOCTL() at netbsd:VOP_IOCTL+0x3b
vn_ioctl() at netbsd:vn_ioctl+0x76
sys_ioctl() at netbsd:sys_ioctl+0x13c
syscall() at netbsd:syscall+0xc4
there are some obvious locking problems to do with "ipf_tokens"
in the version of IPF in the 6.x branch, this lock can be unlocked
when it's not held. does the attached patch look correct?
(the more recent version of IPF in -current doesn't have these problems)
-Chuck

Chuck,
Thanks for picking that up.
You're right in that the comment is wrong.
Cheers,
Darren

great, thanks for confirming. I've requested the patch be applied to 6.x.
I looked at the version in 5.x, it looks like that version works the way
the comment describes so these changes are not needed there either.

-Chuck

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de