[clearview-discuss] [crossbow-discuss] [networking-discuss] BPF/PF_PACKET Code review

[Darren Reed]

On 14/08/09 12:58 AM, Eric Cheng wrote:
> On Thu, Aug 13, 2009 at 10:04:01PM -0700, Darren Reed wrote:
>>> dls_link.c:
>>>
>>> 930:
>>> dls_link_getzid() seems to be unused.
>>>  
>> Discuss.
>> In testing, zone_access_datalink() didn't work as expected, thus it has
>> been thrown to /dev/null and dls_link_getzid() used instead (code path
>> now works as expected.)
>>
>
> I'm fine with keeping dls_link_getzid() if it's needed but do you know
> if the zone_access_datalink() problem is related to the macname issue
> I mentioned in sockmod_pfp:1223?
>
>>> mac_client.c:
>>>
>>> 3253-3254:
>>> I think you can't assume that the mp_copy is still valid after
>>> invoking mpip->mpi_fn(). mp_copy could already be freed.
>>>  
>> Discuss.
>> The basis of "mp_copy == mp" is the mpi_no_copy being true.
>> Users of the interface that do not request the mblk to be copied
>> (by using MAC_PROMISC_FLAGS_NO_COPY) are not entitled
>> to modify the mblk_t that is passed in. That includes prohibition
>> from free'ing them. Or is there some other code path that will
>> free the mblk_t whilst mpi_fn() is being called?
>>
>
> no, it won't get freed.
> I just think it's better to not change the expected behavior of
> mpi_fn() because someone passed in a certain flag. I think it's
> more consistent with other mac callbacks to let it consume the
> packet.
>
> If you're concerned about copying cost, you could do dupmsg() instead
> of copymsg() if mpi_no_copy is set.

No. The entire point of the flag is to avoid any duplication,
be it dupmsg or copymsg. They're simply not required with BPF
and do nothing except making things run slower. When using BPF
to capture packets, there should be no need to allocate any
new data structures in the process of capturing the packet
and the only time packet data is copied is when it matches the
filter being applied to the network traffic.


>>> sockmod_pfp.c:
>>>
>>> 1211:
>>> you should consider adding a pfp_close() function symmetrical
>>> to pfp_open_index() for closing mh/mch.
>>>  
>> Reject.
>> There would only be one use of pfp_close(), whilst there are multiple
>> places that need pfp_open_index()
>>
>
> I don't mind if you keep it this way.
> but from cscope, I see two uses of pfp_open_index(), in both these
> cases I see mac_client_close()/mac_close() as well. so you should
> have more than one use of pfp_close().
>
>>> 716,719:
>>> if you did the pfp_open_index() at 660, these conditions must
>>> be true. you could set a boolean at 660 and use it instead
>>> of checking mh/mch. something like:
>>>
>>> if (new_open) {
>>> 	ASSERT(mch != ps->ps_mch);
>>> 	ASSERT(mh != ps->ps_mh);
>>> 	pfp_close(mch, mh);
>>> }
>>>  
>> Discuss.
>> So you only wanted pfp_close() to do the mac_client_close and the
>> mac_close? That seems hardly worth it... but the new_open change
>> is worthwhile.
>>
>
> I found two cases but as I said I don't mind either way.

Accept.
So with the above change to use "new_open", it does become
worthwhile to have pfp_close().


>>> 330:
>>> you need to specify MAC_PROMISC_FLAGS_VLAN_TAG_STRIP in
>>> mac_promisc_add() otherwise the mhi_bindsap here will be
>>> incorrect for vlan packets.
>>>  
>> Discuss.
>> Does this comment apply to all uses of mac_promisc_add()?
>> I couldn't find any that are at line 330...I think you mean 1330?
>>
>
> I was talking about this:
>
> if ((ps->ps_proto != 0) && (ps->ps_proto != hdr.mhi_bindsap)) {
>
> }
>
> e.g.:
> if someone binds to the ip sap, all inbound packets with a
> vlan header will be dropped.
>
> yes, this affects all calls to mac_promisc_add().
>
> an alternative is to not strip the vlan tag, and skip to the real
> sap yourself, like dls_link_header_info().
>
> I'm not sure what's the correct semantics for pf_packet. it maybe
> better to pass up a full raw header.

Hmmm!
What would be nice is a way to tell mac_header_info() to optionally
iterate through layer 2 headers until the last one is found and
extract the "real" SAP into mhi_bindsap. Looking at
dls_link_header_info(), I can recall looking at it. In its present
form, it isn't useful because it requires a dls_link_t *. But
changing it to use mac_handle_t is quite easy (see attached) and
that would make it possible to use that function rather than writing
another version of it. But changing the first parameter of
dls_link_header_info() implies that the function name and location
should also change...which is getting outside of the scope of what
this project is about. The functional requirement is to provide the
raw ethernet packet.

So I think what I'll do is:
1) change the code to mimic dls_link_header_info()
2) leave the code as is (without the MAC_PROMISC_FLAGS_VLAN_TAG_STRIP
   flag)
3) file a bug that mentions dls_link_header_info() can work with
   a mac_handle_t instead of a dls_link_t * and when the mac/dls
   cleanup gets into gear, it can take care of both pieces of
   code then. See CR#6872007

Darren

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.opensolaris.org/pipermail/clearview-discuss/attachments/20090814/ab833238/attachment.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: dlp_to_mh.patch.txt
URL: <http://mail.opensolaris.org/pipermail/clearview-discuss/attachments/20090814/ab833238/attachment.txt>