Re: [vpp-dev] ACL Plugin: check for null session
Dear Andrew Unfortunately I can't reproduce this case. It's really a rare situation. Regards On Tue, Dec 12, 2017 at 5:43 PM, khers wrote: > Dear Andrew > > This is a good explanation of how session add and delete works, > I think this not a benign operation, I could produce the rare scenario you > explained. I will send backtrace and other details tomorrow. > > On Tue, Dec 12, 2017 at 2:46 PM, Andrew 👽 Yourtchenko > wrote: > >> Dear Khers, >> >> I think you are right. Normally the entry in the session hash table is >> deleted before any operations with the per-worker pool, so we should >> not end up on that line. Also, the deletion itself usually happens as >> a result of the idle timeout - meaning, no packets hit the session for >> a comparatively very long time. Also, the deletion happens in the >> interrupt handler on the worker, so that worker will not be able to >> use that session anyway. The only theoretically possible scenario I >> can see this happening is if the interrupt handler on the worker >> owning the session starts the process of its deletion, and then the >> worker handling the other leg of the connection receives the packet >> just in time to do the lookup of the session in the global table just >> before it gets deleted *AND* then the per-worker session deletion >> happens earlier than we get the pointer for the session. Initially I >> did not have the check when getting the pointer to the session, so in >> this rare case we would reset the timeout or update the flags on the >> free session. So this would be a benign operation. But as some of the >> places used the ~0 as index, that was a bit problematic so i have >> added that check. Coincidentally this is also the exact place which >> appears to have triggered the other issue that you saw. >> >> So I think I might just simplify the check to ensure the index is >> within the bounds of the allowed indices for the pool. >> >> Hope this clarifies the logic... >> >> --a >> >> On 12/11/17, khers wrote: >> > Dear Andrew >> > >> > I'm working on d594711a5d79859a7d0bde83a516f7ab52051d9b commit on >> > stable/1710 branch. sorry for less info. >> > I can't reproduce last issue I have reported, forgot the commit I were >> > working on. >> > >> > Regards, >> > Khers >> > >> > On Mon, Dec 11, 2017 at 12:24 PM, Andrew Yourtchenko < >> ayour...@gmail.com> >> > wrote: >> > >> >> Dear Khers, >> >> >> >> At least the exact commit# you are working with to get more context >> would >> >> be useful - line 1029 on master points to a call acl_fill_5tuple to >> me... >> >> >> >> Also, I have not heard - were you able to reproduce the issue you >> >> contacted about a while ago ? >> >> >> >> --a >> >> >> >> > On 11 Dec 2017, at 08:46, khers wrote: >> >> > >> >> > Dear VPP folks, >> >> > >> >> > The get_session_ptr function may return null pointer, while we do not >> >> check this situation in code, for example fa_node.c line 1029, if the >> >> sess >> >> equals null, we get segmentation fault in next usage of sess. >> >> > Please share your thought about this. >> >> > >> >> > Regards, >> >> > Khers >> >> > ___ >> >> > vpp-dev mailing list >> >> > vpp-dev@lists.fd.io >> >> > https://lists.fd.io/mailman/listinfo/vpp-dev >> >> >> > >> > > ___ vpp-dev mailing list vpp-dev@lists.fd.io https://lists.fd.io/mailman/listinfo/vpp-dev
Re: [vpp-dev] ACL Plugin: check for null session
Dear Andrew This is a good explanation of how session add and delete works, I think this not a benign operation, I could produce the rare scenario you explained. I will send backtrace and other details tomorrow. On Tue, Dec 12, 2017 at 2:46 PM, Andrew 👽 Yourtchenko wrote: > Dear Khers, > > I think you are right. Normally the entry in the session hash table is > deleted before any operations with the per-worker pool, so we should > not end up on that line. Also, the deletion itself usually happens as > a result of the idle timeout - meaning, no packets hit the session for > a comparatively very long time. Also, the deletion happens in the > interrupt handler on the worker, so that worker will not be able to > use that session anyway. The only theoretically possible scenario I > can see this happening is if the interrupt handler on the worker > owning the session starts the process of its deletion, and then the > worker handling the other leg of the connection receives the packet > just in time to do the lookup of the session in the global table just > before it gets deleted *AND* then the per-worker session deletion > happens earlier than we get the pointer for the session. Initially I > did not have the check when getting the pointer to the session, so in > this rare case we would reset the timeout or update the flags on the > free session. So this would be a benign operation. But as some of the > places used the ~0 as index, that was a bit problematic so i have > added that check. Coincidentally this is also the exact place which > appears to have triggered the other issue that you saw. > > So I think I might just simplify the check to ensure the index is > within the bounds of the allowed indices for the pool. > > Hope this clarifies the logic... > > --a > > On 12/11/17, khers wrote: > > Dear Andrew > > > > I'm working on d594711a5d79859a7d0bde83a516f7ab52051d9b commit on > > stable/1710 branch. sorry for less info. > > I can't reproduce last issue I have reported, forgot the commit I were > > working on. > > > > Regards, > > Khers > > > > On Mon, Dec 11, 2017 at 12:24 PM, Andrew Yourtchenko > > > wrote: > > > >> Dear Khers, > >> > >> At least the exact commit# you are working with to get more context > would > >> be useful - line 1029 on master points to a call acl_fill_5tuple to > me... > >> > >> Also, I have not heard - were you able to reproduce the issue you > >> contacted about a while ago ? > >> > >> --a > >> > >> > On 11 Dec 2017, at 08:46, khers wrote: > >> > > >> > Dear VPP folks, > >> > > >> > The get_session_ptr function may return null pointer, while we do not > >> check this situation in code, for example fa_node.c line 1029, if the > >> sess > >> equals null, we get segmentation fault in next usage of sess. > >> > Please share your thought about this. > >> > > >> > Regards, > >> > Khers > >> > ___ > >> > vpp-dev mailing list > >> > vpp-dev@lists.fd.io > >> > https://lists.fd.io/mailman/listinfo/vpp-dev > >> > > > ___ vpp-dev mailing list vpp-dev@lists.fd.io https://lists.fd.io/mailman/listinfo/vpp-dev
Re: [vpp-dev] ACL Plugin: check for null session
Dear Khers, I think you are right. Normally the entry in the session hash table is deleted before any operations with the per-worker pool, so we should not end up on that line. Also, the deletion itself usually happens as a result of the idle timeout - meaning, no packets hit the session for a comparatively very long time. Also, the deletion happens in the interrupt handler on the worker, so that worker will not be able to use that session anyway. The only theoretically possible scenario I can see this happening is if the interrupt handler on the worker owning the session starts the process of its deletion, and then the worker handling the other leg of the connection receives the packet just in time to do the lookup of the session in the global table just before it gets deleted *AND* then the per-worker session deletion happens earlier than we get the pointer for the session. Initially I did not have the check when getting the pointer to the session, so in this rare case we would reset the timeout or update the flags on the free session. So this would be a benign operation. But as some of the places used the ~0 as index, that was a bit problematic so i have added that check. Coincidentally this is also the exact place which appears to have triggered the other issue that you saw. So I think I might just simplify the check to ensure the index is within the bounds of the allowed indices for the pool. Hope this clarifies the logic... --a On 12/11/17, khers wrote: > Dear Andrew > > I'm working on d594711a5d79859a7d0bde83a516f7ab52051d9b commit on > stable/1710 branch. sorry for less info. > I can't reproduce last issue I have reported, forgot the commit I were > working on. > > Regards, > Khers > > On Mon, Dec 11, 2017 at 12:24 PM, Andrew Yourtchenko > wrote: > >> Dear Khers, >> >> At least the exact commit# you are working with to get more context would >> be useful - line 1029 on master points to a call acl_fill_5tuple to me... >> >> Also, I have not heard - were you able to reproduce the issue you >> contacted about a while ago ? >> >> --a >> >> > On 11 Dec 2017, at 08:46, khers wrote: >> > >> > Dear VPP folks, >> > >> > The get_session_ptr function may return null pointer, while we do not >> check this situation in code, for example fa_node.c line 1029, if the >> sess >> equals null, we get segmentation fault in next usage of sess. >> > Please share your thought about this. >> > >> > Regards, >> > Khers >> > ___ >> > vpp-dev mailing list >> > vpp-dev@lists.fd.io >> > https://lists.fd.io/mailman/listinfo/vpp-dev >> > ___ vpp-dev mailing list vpp-dev@lists.fd.io https://lists.fd.io/mailman/listinfo/vpp-dev
Re: [vpp-dev] ACL Plugin: check for null session
Dear Andrew I'm working on d594711a5d79859a7d0bde83a516f7ab52051d9b commit on stable/1710 branch. sorry for less info. I can't reproduce last issue I have reported, forgot the commit I were working on. Regards, Khers On Mon, Dec 11, 2017 at 12:24 PM, Andrew Yourtchenko wrote: > Dear Khers, > > At least the exact commit# you are working with to get more context would > be useful - line 1029 on master points to a call acl_fill_5tuple to me... > > Also, I have not heard - were you able to reproduce the issue you > contacted about a while ago ? > > --a > > > On 11 Dec 2017, at 08:46, khers wrote: > > > > Dear VPP folks, > > > > The get_session_ptr function may return null pointer, while we do not > check this situation in code, for example fa_node.c line 1029, if the sess > equals null, we get segmentation fault in next usage of sess. > > Please share your thought about this. > > > > Regards, > > Khers > > ___ > > vpp-dev mailing list > > vpp-dev@lists.fd.io > > https://lists.fd.io/mailman/listinfo/vpp-dev > ___ vpp-dev mailing list vpp-dev@lists.fd.io https://lists.fd.io/mailman/listinfo/vpp-dev
Re: [vpp-dev] ACL Plugin: check for null session
Dear Khers, At least the exact commit# you are working with to get more context would be useful - line 1029 on master points to a call acl_fill_5tuple to me... Also, I have not heard - were you able to reproduce the issue you contacted about a while ago ? --a > On 11 Dec 2017, at 08:46, khers wrote: > > Dear VPP folks, > > The get_session_ptr function may return null pointer, while we do not check > this situation in code, for example fa_node.c line 1029, if the sess equals > null, we get segmentation fault in next usage of sess. > Please share your thought about this. > > Regards, > Khers > ___ > vpp-dev mailing list > vpp-dev@lists.fd.io > https://lists.fd.io/mailman/listinfo/vpp-dev ___ vpp-dev mailing list vpp-dev@lists.fd.io https://lists.fd.io/mailman/listinfo/vpp-dev
