Mailing List Archive

Hi Amir
what is the file location you are talking about?

Alfredo

> On 29 May 2017, at 18:23, Amir Kaduri <akaduri75@gmail.com> 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

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
<cardigliano@ntop.org> wrote:
> Hi Amir
> what is the file location you are talking about?
>
> Alfredo
>
>> On 29 May 2017, at 18:23, Amir Kaduri <akaduri75@gmail.com> 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

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 <akaduri75@gmail.com> 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
> <cardigliano@ntop.org> wrote:
>> Hi Amir
>> what is the file location you are talking about?
>>
>> Alfredo
>>
>>> On 29 May 2017, at 18:23, Amir Kaduri <akaduri75@gmail.com> 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

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
<cardigliano@ntop.org> 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 <akaduri75@gmail.com> 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
>> <cardigliano@ntop.org> wrote:
>>> Hi Amir
>>> what is the file location you are talking about?
>>>
>>> Alfredo
>>>
>>>> On 29 May 2017, at 18:23, Amir Kaduri <akaduri75@gmail.com> 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

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 <akaduri75@gmail.com> 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
> <cardigliano@ntop.org> 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 <akaduri75@gmail.com> 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
>>> <cardigliano@ntop.org> wrote:
>>>> Hi Amir
>>>> what is the file location you are talking about?
>>>>
>>>> Alfredo
>>>>
>>>>> On 29 May 2017, at 18:23, Amir Kaduri <akaduri75@gmail.com> 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

Ok, thanks.

On Thu, Jun 1, 2017 at 12:16 PM, Alfredo Cardigliano
<cardigliano@ntop.org> 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 <akaduri75@gmail.com> 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
>> <cardigliano@ntop.org> 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 <akaduri75@gmail.com> 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
>>>> <cardigliano@ntop.org> wrote:
>>>>> Hi Amir
>>>>> what is the file location you are talking about?
>>>>>
>>>>> Alfredo
>>>>>
>>>>>> On 29 May 2017, at 18:23, Amir Kaduri <akaduri75@gmail.com> 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