Re: close(2) while accept(2) is blocked
on 30/03/2013 18:14 John-Mark Gurney said the following: As someone else pointed out in this thread, if a userland program depends upon this behavior, it has a race condition in it... Thread 1 Thread 2Thread 3 enters routine to read enters routine to close calls close(3) open() returns 3 does read(3) for orignal fd How can the original threaded program ensure that thread 2 doesn't create a new fd in between? So even if you use a lock, this won't help, because as far as I know, there is no enter read and unlock mutex call yet... I decided long ago that this is only solvable by proper use of locking and ensuring that if you call close (the syscall), that you do not have any other thread that may use the fd. It's the close routine's (not syscall) function to make sure it locks out other threads and all other are out of the code path that will use the fd before it calls close.. If someone could describe how this new eject a person from read could be done in a race safe way, then I'd say go ahead w/ it... Otherwise we're just moving the race around, and letting people think that they have solved the problem when they haven't... I think I remeber another thread about this from a year or two ago, but I couldn't find it... If someone finds it, posting a link would be nice.. I wish to abstract as much as possible from how an application may use, misuse or even abuse the close+ interaction. But I think that the behavior that provides more information / capabilities is preferable over the behavior that does not. E.g. your example above does not apply to a utility that has only two threads. The three threads problem can also be solved if all the threads cooperate. But as I've said. -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: call suspend_cpus() under smp_ipi_mtx
on 01/04/2013 17:52 John Baldwin said the following: Hmm, I think intr_table_lock used to be a spin lock at some point. I don't remember why we changed it to a regular mutex. It may be that there was a lock order reason for that. :( I came up with the following patch: http://people.freebsd.org/~avg/intr_table_lock.diff Please note the witness change. Part of it is to prepare for smp_ipi_mtx - intr_table_lock order, but I also had to move entropy harvest mutex because it is used with msleep_spin. Also intr_table_lock interacts with sched lock. This seems to work without problems or any warnings with WITNESS !WITNESS_SKIPSPIN, but it is very possible that I am not exercising all the relevant code paths. P.S. Looking through history it seems that in r169391 intr_table_lock was changed from a spinlock to a sx lock (later it was changed to the current mutex). The commit message explains why spinlock was not needed, but it doesn't seem to say why it was unacceptable: Minor fixes and tweaks to the x86 interrupt code: - Split the intr_table_lock into an sx lock used for most things, and a spin lock to protect intrcnt_index. Originally I had this as a spin lock so interrupt code could use it to lookup sources. However, we don't actually do that because it would add a lot of overhead to interrupts, and if we ever do support removing interrupt sources, we can use other means to safely do so w/o locking in the interrupt handling code. - Replace is_enabled (boolean) with is_handlers (a count of handlers) to determine if a source is enabled or not. This allows us to notice when a source is no longer in use. When that happens, we now invoke a new PIC method (pic_disable_intr()) to inform the PIC driver that the source is no longer in use. The I/O APIC driver frees the APIC IDT vector when this happens. The MSI driver no longer needs to have a hack to clear is_enabled during msi_alloc() and msix_alloc() as a result of this change as well. - Add an apic_disable_vector() to reset an IDT vector back to Xrsvd to complement apic_enable_vector() and use it in the I/O APIC and MSI code when freeing an IDT vector. - Add a new nexus hook: nexus_add_irq() to ask the nexus driver to add an IRQ to its irq_rman. The MSI code uses this when it creates new interrupt sources to let the nexus know about newly valid IRQs. Previously the msi_alloc() and msix_alloc() passed some extra stuff back to the nexus methods which then added the IRQs. This approach is a bit cleaner. - Change the MSI sx lock to a mutex. If we need to create new sources, drop the lock, create the required number of sources, then get the lock and try the allocation again. -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: close(2) while accept(2) is blocked
on 01/04/2013 18:22 John Baldwin said the following: I think you need to split the 'struct file' reference count into two different counts similar to the how we have vref/vrele vs vhold/vdrop for vnodes. The fget for accept and probably most other system calls should probably be equivalent to vhold, whereas things like open/dup (and storing an fd in a cmsg) should be more like vref. close() should then be a vrele(). This model makes perfect sense. Unfortunately, ENOTIME to work on this. Meanwhile I am using the following patch specific to local domain sockets, accept(2) and shutdown(2). Turns out that the problematic application does both shutdown(RDWR) and close(2), but that doesn't help on FreeBSD. BTW, this is the application: http://thread.gmane.org/gmane.os.freebsd.devel.office/1754 The patch does help. Author: Andriy Gapon a...@icyb.net.ua Date: Thu Mar 28 20:08:13 2013 +0200 [test!] uipc_shutdown: use soisdisconnected instead of socantsendmore So that in addition to disabling sends we also wake up threads blocked in accept (on so_timeo in general). diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c index 9b60eab..e93d46c 100644 --- a/sys/kern/uipc_usrreq.c +++ b/sys/kern/uipc_usrreq.c @@ -1074,7 +1074,7 @@ uipc_shutdown(struct socket *so) UNP_LINK_WLOCK(); UNP_PCB_LOCK(unp); - socantsendmore(so); + soisdisconnected(so); unp_shutdown(unp); UNP_PCB_UNLOCK(unp); UNP_LINK_WUNLOCK(); -- Andriy Gapon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
extend printf(9) %b format to allow 0 bits set
A small patch from my wanderings today for your review. This patch to kvprintf() allows me to set a %b format string of: \20\0unset\1ONESET\2TWOSET In the case that the variable being compared has the value of 0, it will display 0x0unset http://people.freebsd.org/~sbruno/subr_prf.txt Sean signature.asc Description: This is a digitally signed message part
Re: CFR: FireWire: Don't allow a tlabel to reference an xfer after free
On Thu, 2013-03-28 at 11:25 -0600, Will Andrews wrote: Diff: http://people.freebsd.org/~will/patches/fix-fwmem-use-after-free.diff From the commit log: FireWire: Don't allow a tlabel to reference an xfer after free. sys/dev/firewire/firewire.c: - fw_xfer_unload(): Since we are about to free this xfer, call fw_tl_free() to remove the xfer from its tlabel's list, if it has a tlabel. - In every occasion when a xfer is removed from a tlabel's list, reset xfer-tl to -1 while holding fc-tlabel_lock, so that the xfer isn't mis-identified as belonging to a tlabel. Thanks, --Will. ___ Ack. Looks like a valid commit. sean signature.asc Description: This is a digitally signed message part
