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.
- There does not seem to be any flow control on the receive side. It is the
responsibility of the socket module to assert flow control/drop packets in
case the recv upcall returned ENOSPC. Sorry about the lack of
documentation, but have a look at udp_ulp_recv() and udp_clr_flowctrl().
Accepted.
- line 93: Does pfp_kstats need to have global scope? Just declare it in
sockpfp_init.
Rejected.
pfp_kstats doesn't need to have global scope, its role is a template for
assingning
into ks_stats. However, having a structure inside a function with an
assignment array
such as this one would look out of place inside a function.
- line 156: Using PF_PACKET rather than PFP would make for a better
description.
Accepted.
- line 210: empty line.
Accepted.
- lines 217 - 218: Not needed, since kmem_cache_create cannot fail.
Accepted.
- line 240: empty line
Accepted.
- line 276: The error code should be 'EPROTOTYPE'.
Accepted.
- lines 300 - 324: There is not much work in the ctor/dtor. Do you even
need to use a kmem cache for creating pfpsocks?
It does not appear that it is needed, per se, so I'll remove it.
- line 389: Use MBLKL?
Accepted.
- line 419: extra space.
Accepted.
- lines 684, 689: Looks like you are missing some cleanup in these two error
cases. Should you not close the mac and mac client?
Accepted.
- lines 936, 962: The comments for these functions say that 'arg' could
potentially be in user space, so I expect that it can handle kernel
addresses as well. But the code always uses copyin. Should the comment or
code be updated?
In both of these cases, arg is the same as optval: a pointer to a
kernel-space "bounce buffer."
- line 1005: optval points to a kernel buffer, so use bcopy.
Accepted.
- line 1044: optval points to a kernel buffer.
Accepted.
- lines 1139, 1146: optval points to kernel buffers.
Accepted.
- line 1156: memory leak; you do not free fcode if the copyin fails
Accepted.
Darren
_______________________________________________
networking-discuss mailing list
[email protected]
