Re: [Ntop-misc] Consider adding a warning in case ip_version is missing in function hash_pkt()

2017-06-01 Thread Amir Kaduri 
Ok, thanks.

On Thu, Jun 1, 2017 at 12:16 PM, Alfredo Cardigliano
 wrote:
> Hi Amir
> regardless of the user input, when a packet is received  ah hash is computed 
> calling parse_pkt -> parse_raw_pkt -> hash_pkt_header ->  hash_pkt
> if the packet is not an IP packet (ARP or whatever), ip_version is 0 and your 
> message is printed, for every packet.
>
> Alfredo
>
>> On 1 Jun 2017, at 11:00, Amir Kaduri  wrote:
>>
>> Hi Alfredo,
>>
>> I'm aware that this function is called per packet, but if you think of
>> it deeper, then if everything is configured correctly by the user,
>> ip_version should give 4 or 6 only, thus it will never rich the "else"
>> I suggest to add and it will never actually add any redundant check
>> nor print per packet. The only time the warning will be printed is
>> only if ip_version will not be 4 or 6 and its great. It clearly warns
>> about a problem (whether its not set or set to anything else than 4 or
>> 6).
>> Just to make sure you got me right, the code with the fix should look like 
>> that:
>> if (ip_version == 4)
>>hash += host_peer_a.v4 + host_peer_b.v4;
>> else if (ip_version == 6)
>>hash += (<... Omitted long condition >);
>> else
>>printk("Warning: ip_version (currently equals %d) should be 4 or 6
>> only\n", ip_version);
>>
>> So again, if you take a moment to think of it, this warning should
>> never cost you anything if everything is configured well, but it warns
>> if anything went wrong.
>>
>> Thanks,
>> Amir
>>
>> On Thu, Jun 1, 2017 at 11:39 AM, Alfredo Cardigliano
>>  wrote:
>>> Hi Amir
>>> hash_pkt is called per-packet, we want to a void a printk per packet to 
>>> avoid flooding dmesg and slowing doen packet processing,
>>> if ip_version comes from user input, sanity check should happen in 
>>> userspace in my opinion.
>>>
>>> Regards
>>> Alfredo
>>>
 On 1 Jun 2017, at 07:38, Amir Kaduri  wrote:

 Hi Alfredo,

 This is the exact location of the function:
 https://github.com/ntop/PF_RING/blob/dev/kernel/pf_ring.c#L1794

 Thanks,
 Amir

 On Mon, May 29, 2017 at 7:28 PM, Alfredo Cardigliano
  wrote:
> Hi Amir
> what is the file location you are talking about?
>
> Alfredo
>
>> On 29 May 2017, at 18:23, Amir Kaduri  wrote:
>>
>> In function hash_pkt(), there is a if-else-if statement based on
>> ip_version. If ip_version is 0, the hash won't include the ipaddress.
>> Since the ip_version might come from the user input, I suggest adding
>> an "else" and issue a warning in case ip_version wasn't set.
>> ___
>> Ntop-misc mailing list
>> Ntop-misc@listgateway.unipi.it
>> http://listgateway.unipi.it/mailman/listinfo/ntop-misc
>
>
> ___
> Ntop-misc mailing list
> Ntop-misc@listgateway.unipi.it
> http://listgateway.unipi.it/mailman/listinfo/ntop-misc
 ___
 Ntop-misc mailing list
 Ntop-misc@listgateway.unipi.it
 http://listgateway.unipi.it/mailman/listinfo/ntop-misc
>>>
>>>
>>> ___
>>> Ntop-misc mailing list
>>> Ntop-misc@listgateway.unipi.it
>>> http://listgateway.unipi.it/mailman/listinfo/ntop-misc
>> ___
>> Ntop-misc mailing list
>> Ntop-misc@listgateway.unipi.it
>> http://listgateway.unipi.it/mailman/listinfo/ntop-misc
>
>
> ___
> Ntop-misc mailing list
> Ntop-misc@listgateway.unipi.it
> http://listgateway.unipi.it/mailman/listinfo/ntop-misc
___
Ntop-misc mailing list
Ntop-misc@listgateway.unipi.it
http://listgateway.unipi.it/mailman/listinfo/ntop-misc

Re: [Ntop-misc] Consider adding a warning in case ip_version is missing in function hash_pkt()

2017-06-01 Thread Amir Kaduri 
Hi Alfredo,

I'm aware that this function is called per packet, but if you think of
it deeper, then if everything is configured correctly by the user,
ip_version should give 4 or 6 only, thus it will never rich the "else"
I suggest to add and it will never actually add any redundant check
nor print per packet. The only time the warning will be printed is
only if ip_version will not be 4 or 6 and its great. It clearly warns
about a problem (whether its not set or set to anything else than 4 or
6).
Just to make sure you got me right, the code with the fix should look like that:
if (ip_version == 4)
hash += host_peer_a.v4 + host_peer_b.v4;
else if (ip_version == 6)
hash += (<... Omitted long condition >);
else
printk("Warning: ip_version (currently equals %d) should be 4 or 6
only\n", ip_version);

So again, if you take a moment to think of it, this warning should
never cost you anything if everything is configured well, but it warns
if anything went wrong.

Thanks,
Amir

On Thu, Jun 1, 2017 at 11:39 AM, Alfredo Cardigliano
 wrote:
> Hi Amir
> hash_pkt is called per-packet, we want to a void a printk per packet to avoid 
> flooding dmesg and slowing doen packet processing,
> if ip_version comes from user input, sanity check should happen in userspace 
> in my opinion.
>
> Regards
> Alfredo
>
>> On 1 Jun 2017, at 07:38, Amir Kaduri  wrote:
>>
>> Hi Alfredo,
>>
>> This is the exact location of the function:
>> https://github.com/ntop/PF_RING/blob/dev/kernel/pf_ring.c#L1794
>>
>> Thanks,
>> Amir
>>
>> On Mon, May 29, 2017 at 7:28 PM, Alfredo Cardigliano
>>  wrote:
>>> Hi Amir
>>> what is the file location you are talking about?
>>>
>>> Alfredo
>>>
 On 29 May 2017, at 18:23, Amir Kaduri  wrote:

 In function hash_pkt(), there is a if-else-if statement based on
 ip_version. If ip_version is 0, the hash won't include the ipaddress.
 Since the ip_version might come from the user input, I suggest adding
 an "else" and issue a warning in case ip_version wasn't set.
 ___
 Ntop-misc mailing list
 Ntop-misc@listgateway.unipi.it
 http://listgateway.unipi.it/mailman/listinfo/ntop-misc
>>>
>>>
>>> ___
>>> Ntop-misc mailing list
>>> Ntop-misc@listgateway.unipi.it
>>> http://listgateway.unipi.it/mailman/listinfo/ntop-misc
>> ___
>> Ntop-misc mailing list
>> Ntop-misc@listgateway.unipi.it
>> http://listgateway.unipi.it/mailman/listinfo/ntop-misc
>
>
> ___
> Ntop-misc mailing list
> Ntop-misc@listgateway.unipi.it
> http://listgateway.unipi.it/mailman/listinfo/ntop-misc
___
Ntop-misc mailing list
Ntop-misc@listgateway.unipi.it
http://listgateway.unipi.it/mailman/listinfo/ntop-misc

Re: [Ntop-misc] Consider adding a warning in case ip_version is missing in function hash_pkt()

2017-06-01 Thread Alfredo Cardigliano 
Hi Amir
hash_pkt is called per-packet, we want to a void a printk per packet to avoid 
flooding dmesg and slowing doen packet processing,
if ip_version comes from user input, sanity check should happen in userspace in 
my opinion.

Regards
Alfredo

> On 1 Jun 2017, at 07:38, Amir Kaduri  wrote:
> 
> Hi Alfredo,
> 
> This is the exact location of the function:
> https://github.com/ntop/PF_RING/blob/dev/kernel/pf_ring.c#L1794
> 
> Thanks,
> Amir
> 
> On Mon, May 29, 2017 at 7:28 PM, Alfredo Cardigliano
>  wrote:
>> Hi Amir
>> what is the file location you are talking about?
>> 
>> Alfredo
>> 
>>> On 29 May 2017, at 18:23, Amir Kaduri  wrote:
>>> 
>>> In function hash_pkt(), there is a if-else-if statement based on
>>> ip_version. If ip_version is 0, the hash won't include the ipaddress.
>>> Since the ip_version might come from the user input, I suggest adding
>>> an "else" and issue a warning in case ip_version wasn't set.
>>> ___
>>> Ntop-misc mailing list
>>> Ntop-misc@listgateway.unipi.it
>>> http://listgateway.unipi.it/mailman/listinfo/ntop-misc
>> 
>> 
>> ___
>> Ntop-misc mailing list
>> Ntop-misc@listgateway.unipi.it
>> http://listgateway.unipi.it/mailman/listinfo/ntop-misc
> ___
> Ntop-misc mailing list
> Ntop-misc@listgateway.unipi.it
> http://listgateway.unipi.it/mailman/listinfo/ntop-misc



signature.asc
Description: Message signed with OpenPGP
___
Ntop-misc mailing list
Ntop-misc@listgateway.unipi.it
http://listgateway.unipi.it/mailman/listinfo/ntop-misc