On Tue, Jul 21, 2009 at 01:58:18PM -0700, Darren Reed wrote: > Anders Persson wrote: >> On Tue, Jul 21, 2009 at 07:45:36AM -0700, Darren Reed wrote: >> >>> Anders Persson wrote: >>> >>>> On Mon, Jul 06, 2009 at 07:17:14PM -0700, Darren Reed wrote: >>>> >>>>> I'd like to invite people to review the PF_PACKET port of the >>>>> packet capture project. Apologies if some of the Makefile bits >>>>> seem like orphans, other changes are embedded with BPF work >>>>> that isn't yet ready for review. The primary file requiring review >>>>> here is sockmod_pfp.c, but please feel free to review others. >>>>> I'd like to receieve any comments by the end of next week >>>>> (17th of July 2009.) >>>>> >>>>> http://cr.opensolaris.org/~darrenr/pf_packet_review/ >>>>> >>>> Hi Darren, >>>> >>>> The comments below are for sockmod_pfp.c. >>>> >>>> - There are a few code paths that use copyin/copyout explicitly, making >>>> them >>>> unsuitable for kernel sockets. Is it expected that PF_PACKET will only be >>>> used by userland sockets? There is unfortunately no mechanism for marking >>>> a module as "unavailable" for kernel consumers, which is maybe what is >>>> needed in this case. >>>> >>> Discuss. >>> >>> It would seem that ddi_copyin/out are normally used in this circumstance, >>> however the prototype for the sockmod ioctl call does not provide a flags >>> parameter, like the normal driver ioctl entry point does, meaning >>> there is no >>> easy way to communicate to the copy function whether the source is in user >>> or kernel space. See ddi_copyin(9f). Fixing this problem properly requires >>> a revision to the socket module API's socksetopt/getsockopt interfaces. >>> >> >> Well, the ioctl sockmod entry point has a 'mode' flag, just like the >> driver entry point. So checking for FKIOCTL will tell you whether the >> buffer is a kernel address. However, that does not exist for >> {set,get}sockopt(). Do you already need to "tweak" applications that >> use LSF for them to use PF_PACKET on OpenSolaris? If so, would it be >> possible to have the application specify the total size of the filter >> when calling SO_ATTACH_FILTER? >> > > The goal of this project is to not require developers to need to do > extra tweaking > to target Solaris - that defeats the purpose of developing a compatible API. > > This is starting to sound like an RFE for sockfs to modify > get/setsockopt entry > points for socket modules.
Potentially, yes. The way SO_ATTACH_FILTER is handled is somewhat unusual (as far as socket options goes), but I guess it mimics BIOCSETF. One more comment on the code: - line 1155: Should you not make sure that the filter size is not outrageous before allocating space for it? Cheers, Anders _______________________________________________ networking-discuss mailing list [email protected]
